Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wasi: move Readdir logics from fdReaddirFn to Readdir methods #1483

Merged
merged 1 commit into from
May 23, 2023

Conversation

evacchi
Copy link
Contributor

@evacchi evacchi commented May 19, 2023

As suggested in this comment, the logic that populates the fields in Readdir is moved to methods of the Readdir struct. We are not exporting an interface yet.

  • The fields are no longer exported.
  • There are new fields that are necessary to maintain the position of the cursor, and func fields to avoid embedding a FileEntry.
  • We export 2 constructors: one takes a FileEntry, and it is defined in terms of the other; the other takes 2 funcs dirInit and dirReader that are invoked when the initializing, resetting, advancing the cursor and filling the internal buffer.

Notably, for simplicity, entries are read in batches; dirents is always at most large direntBufSize = 16 (currently), which is usually the (fixed) size of each batch, unless it is the initial fetch, or it is the last fetch:

  • the initial fetch is dirReader(n), where n := direntBufSize - len(initial), and len(initial) is the size of the slice returned by dirInit()
  • all fetches may return len(batch) <= direntBufSize; if len(dirent) < direntBufSize, then we have reached the end of the listing for this directory.

The cursor/iterator-like API has Peek(), Advance() methods. It also provides Skip(n), Reset(), Cookie() and Rewind(cookie).

  • Advance() advances the cursor field and the countRead field. The countRead is the absolute index of the current item; cursor is the index in the current window (dirent)
  • Rewind(cookie) allows seeking backwards to the position of the given cookie. The cookie corresponds to the internal countRead. When the cookie is 0, Rewind(0) is equivalent to Reset(). It is not possible to seek forward (beyond the cursor). It is not possible to seek backwards to a value outside the current window.
  • Skip(n) is mostly used for testing, the other methods are more useful and used in fdReaddirFn too, and it is a short-hand for invoking Advance() n times.

Tests have been migrated to unit tests of Readdir when it made sense, and they were kept in the wasi namespace when it was simpler.

@evacchi evacchi marked this pull request as draft May 19, 2023 19:38
@evacchi
Copy link
Contributor Author

evacchi commented May 19, 2023

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `0`,
 right: `1`: expected one entry', src/bin/fd_readdir.rs:167:5

eh fair enough 🤷 😛

@evacchi evacchi force-pushed the readdir-methods-refactor branch from 0991808 to 33d423a Compare May 22, 2023 16:16
@evacchi evacchi marked this pull request as ready for review May 22, 2023 16:52
Copy link
Contributor

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely getting cleaner! let's make sure we retain sliding window so that a huge directory doesn't OOM. Ack it is more work, but worth it.

@evacchi
Copy link
Contributor Author

evacchi commented May 23, 2023

Sure, will do the sliding window (in fact a few commits earlier it was there), but I would keep the window fixed (direntBufSize) instead of using the user request value, that should make it simpler. Unless we have a reason for that too

@evacchi evacchi force-pushed the readdir-methods-refactor branch from 5529cde to 9273e23 Compare May 23, 2023 16:21
@evacchi
Copy link
Contributor Author

evacchi commented May 23, 2023

restored the sliding window, cleaned up, squashed, rebased

Verified

This commit was signed with the committer’s verified signature.
blink1073 Steven Silvester
The logic that populates the fields in `Readdir` is moved to methods of
the `Readdir` struct. We are not exporting an interface yet.

- The fields are no longer exported.
- There are new fields that are necessary to maintain the position
  of the cursor, and `func` fields to avoid embedding a `FileEntry`.
- We export 2 constructors: one takes a `FileEntry`, and it is defined
  in terms of the other; the other takes 2 `func`s `dirInit` and
  `dirReader` that are invoked when the initializing, resetting,
  advancing the cursor and filling the internal buffer.

Notably, for simplicity, entries are read in batches; `dirents` is
always at most large `direntBufSize`, which is usually the (fixed) size
of each batch, unless it is the initial fetch, or it is the last fetch:
- the initial fetch is `dirReader(n)`, where `n := direntBufSize - len(initial)`,
  and `len(initial)` is the size of the slice returned by `dirInit()`
- all fetches may return `len(batch) <= direntBufSize`;
  if `len(dirent) < direntBufSize`, then we have reached the
  end of the listing for this directory.

The cursor/iterator-like API has `Peek()`, `Advance()` methods.
It also provides `Skip(n)`, `Reset()`, `Cookie()` and `Rewind(cookie)`.

- `Advance()` advances the `cursor` field and the `countRead` field.
  The `countRead` is the absolute index of the current item;
  `cursor` is the index in the current window (`dirent`)
- `Rewind(cookie)` allows seeking backwards to the position of the
  given cookie. The cookie corresponds to the internal `countRead`.
  When the cookie is 0, `Rewind(0)` is equivalent to `Reset()`.
  It is not possible to seek forward (beyond the cursor). It is not
  possible to seek backwards to a value outside the current window.
- `Skip(n)` is mostly used for testing, the other methods are more
  useful and used in `fdReaddirFn` too, and it is a short-hand
  for invoking `Advance()` `n` times.

Tests have been migrated to unit tests of `Readdir` when it made sense,
and they were kept in the wasi namespace when it was simpler.

Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
@evacchi evacchi force-pushed the readdir-methods-refactor branch from 9273e23 to 8187143 Compare May 23, 2023 16:28
Copy link
Contributor

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so much cleaner than what I had before. Great job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants