Skip to content

Add skip() method to Source interface to allow efficient seeks #14291

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.

Sign up for GitHub

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

Merged
edolstra merged 3 commits into from
Oct 20, 2025
Merged

Add skip() method to Source interface to allow efficient seeks #14291

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
daa7e0d
Source: Add skip() method
edolstra Oct 17, 2025
67bffa1
NullFileSystemObjectSink: Skip over file contents
edolstra Oct 17, 2025
6c9083d
Use a smaller buffer
edolstra Oct 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/libutil/archive.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,11 @@ static void parseContents(CreateRegularFileSink & sink, Source & source)

sink.preallocateContents(size);

if (sink.skipContents) {
source.skip(size + (size % 8 ? 8 - (size % 8) : 0));
Copy link
Contributor

@xokdvium xokdvium Oct 20, 2025

Choose a reason for hiding this comment

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

This just does an alignUp with alignment = 8? Seems like a useful utility that we could move to libutil in a follow-up?

Copy link
Contributor

@fzakaria fzakaria Oct 20, 2025

Choose a reason for hiding this comment

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

(for my own edification) why does it align on 8 byte boundaries ?
Is too specific to x86_64 ?

What about other ISAs or 32bit.

Copy link
Contributor

@xokdvium xokdvium Oct 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

NARs pad to 8 bytes

Copy link
Contributor

@fzakaria fzakaria Oct 20, 2025

Choose a reason for hiding this comment

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

Okay you will hate this idea; but let's write a spec for https://kaitai.io/#what-is-it for fun as a side-project.

Copy link
Member Author

@edolstra edolstra Oct 20, 2025

Choose a reason for hiding this comment

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

Yes, that would be useful as a utility.

Copy link
Contributor

@xokdvium xokdvium Oct 20, 2025

Choose a reason for hiding this comment

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

Done in #14305

Copy link
Member

@Ericson2314 Ericson2314 Oct 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Okay you will hate this idea; but let's write a spec for https://kaitai.io/#what-is-it for fun as a side-project.

https://chatgpt.com/share/68f6a47c-add8-800c-9bcc-bf17799ea0f3 hope it can be improved! from a quickest of glances that has some cruft that ought not to be there

Copy link
Contributor

@fzakaria fzakaria Oct 20, 2025

Choose a reason for hiding this comment

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

I'll work on this tonight if you want to hack on it.

Copy link
Member

@Ericson2314 Ericson2314 Oct 20, 2025

Choose a reason for hiding this comment

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

I can't join tonight, but yes I do think that would be a nice complement to the JSON Schema stuff @roberth and I have been doing.

return;
}

uint64_t left = size;
std::array<char, 65536> buf;

Expand Down Expand Up @@ -166,7 +171,7 @@ static void parse(FileSystemObjectSink & sink, Source & source, const CanonPath
auto expectTag = [&](std::string_view expected) {
auto tag = getString();
if (tag != expected)
throw badArchive("expected tag '%s', got '%s'", expected, tag);
throw badArchive("expected tag '%s', got '%s'", expected, tag.substr(0, 1024));
};

expectTag("(");
Expand Down
2 changes: 2 additions & 0 deletions src/libutil/fs-sink.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,8 @@ void NullFileSystemObjectSink::createRegularFile(
void isExecutable() override {}
} crf;

crf.skipContents = true;

// Even though `NullFileSystemObjectSink` doesn't do anything, it's important
// that we call the function, to e.g. advance the parser using this
// sink.
Expand Down
8 changes: 8 additions & 0 deletions src/libutil/include/nix/util/fs-sink.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,14 @@ namespace nix {
*/
struct CreateRegularFileSink : Sink
{
/**
* If set to true, the sink will not be called with the contents
* of the file. `preallocateContents()` will still be called to
* convey the file size. Useful for sinks that want to efficiently
* discard the contents of the file.
*/
bool skipContents = false;

virtual void isExecutable() = 0;

/**
Expand Down
5 changes: 5 additions & 0 deletions src/libutil/include/nix/util/serialise.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,8 @@ struct Source
void drainInto(Sink & sink);

std::string drain();

virtual void skip(size_t len);
};

/**
Expand Down Expand Up @@ -177,6 +179,7 @@ struct FdSource : BufferedSource
Descriptor fd;
size_t read = 0;
BackedStringView endOfFileError{"unexpected end-of-file"};
bool isSeekable = true;

FdSource()
: fd(INVALID_DESCRIPTOR)
Expand All @@ -200,6 +203,8 @@ struct FdSource : BufferedSource
*/
bool hasData();

void skip(size_t len) override;

protected:
size_t readUnbuffered(char * data, size_t len) override;
private:
Expand Down
48 changes: 45 additions & 3 deletions src/libutil/serialise.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,9 +94,8 @@ void Source::drainInto(Sink & sink)
{
std::array<char, 8192> buf;
while (true) {
size_t n;
try {
n = read(buf.data(), buf.size());
auto n = read(buf.data(), buf.size());
sink({buf.data(), n});
} catch (EndOfFile &) {
break;
Expand All @@ -111,6 +110,16 @@ std::string Source::drain()
return std::move(s.s);
}

void Source::skip(size_t len)
{
std::array<char, 8192> buf;
while (len) {
auto n = read(buf.data(), std::min(len, buf.size()));
assert(n <= len);
len -= n;
}
}

size_t BufferedSource::read(char * data, size_t len)
{
if (!buffer)
Expand All @@ -120,7 +129,7 @@ size_t BufferedSource::read(char * data, size_t len)
bufPosIn = readUnbuffered(buffer.get(), bufSize);

/* Copy out the data in the buffer. */
size_t n = len > bufPosIn - bufPosOut ? bufPosIn - bufPosOut : len;
auto n = std::min(len, bufPosIn - bufPosOut);
memcpy(data, buffer.get() + bufPosOut, n);
bufPosOut += n;
if (bufPosIn == bufPosOut)
Expand Down Expand Up @@ -191,6 +200,39 @@ bool FdSource::hasData()
}
}

void FdSource::skip(size_t len)
{
/* Discard data in the buffer. */
if (len && buffer && bufPosIn - bufPosOut) {
if (len >= bufPosIn - bufPosOut) {
len -= bufPosIn - bufPosOut;
bufPosIn = bufPosOut = 0;
} else {
bufPosOut += len;
len = 0;
}
}

#ifndef _WIN32
/* If we can, seek forward in the file to skip the rest. */
if (isSeekable && len) {
if (lseek(fd, len, SEEK_CUR) == -1) {
if (errno == ESPIPE)
isSeekable = false;
else
throw SysError("seeking forward in file");
} else {
read += len;
return;
}
}
#endif

/* Otherwise, skip by reading. */
if (len)
BufferedSource::skip(len);
}

size_t StringSource::read(char * data, size_t len)
{
if (pos == s.size())
Expand Down
Loading