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

Streaming: Don't load the entire file into memory #17

Merged
merged 5 commits into from May 17, 2015
Merged

Streaming: Don't load the entire file into memory #17

merged 5 commits into from May 17, 2015

Conversation

feross
Copy link
Collaborator

@feross feross commented May 15, 2015

Before this change, FileReadStream would read the full file into memory
with a single readAsArrayBuffer() call.

This commit calls readAsArrayBuffer() with a slice of the file blob for
each call to _read(). This means the blob is never fully loaded into
memory.

The same FileReader object is reused, just with different slices of the
file blob passed into readAsArrayBuffer().

Also, this eliminates the event named 'readable' which was causing bugs
when I piped this stream into another stream library.


I also added browser tests for FileReadStream.

Run the tests in a local browser with npm run test-local. Run the tests remotely with a free sauce labs account with npm test. Also, if we set up travis, it can run the tests automatically for us. :-)

run the tests in a local browser with `npm run test-local`.

run the tests remotely with a free sauce labs account with `npm test`.
Before this change, FileReadStream would read the full file into memory
with a single readAsArrayBuffer() call.

This commit calls readAsArrayBuffer() with a slice of the file blob for
each call to _read(). This means the blob is never fully loaded into
memory.

The same FileReader object is reused, just with different slices of the
file blob passed into readAsArrayBuffer().

Also, this eliminates the event named 'readable' which was causing bugs
when I piped this stream into another stream library.
Chrome has memory leaks in FileReader
(https://code.google.com/p/chromium/issues/detail?id=114548).

200KB chunks seems to be a good balance between speed and memory
utilization, from my testing. We can continue to tweak this as things
change.
@DamonOehlman
Copy link
Owner

Thanks heaps mate - I've been meaning to do this for some time, but keep getting distracted with other things. I'll merge this change and I've made you a collaborator (and also give you npm publish access) for future releases.

DamonOehlman added a commit that referenced this pull request May 17, 2015
	Streaming: Don't load the entire file into memory
@DamonOehlman DamonOehlman merged commit 5137ab8 into DamonOehlman:master May 17, 2015
@DamonOehlman
Copy link
Owner

@feross I made the decision to bump this to version 4.0.0 as because I hadn't had tests for this prior (thanks for introducing them - I'll get Travis integrated soon) if there were any changes that affected people with the new slice behaviour it would be best if they intentionally upgraded to filestream@4 rather than it just happening.

@nathanoehlman This is behaviour I was talking to you about looking into for the stuff you have been using filestream for. I would recommend upgrading to this new version and seeing if you notice any improvements :)

@feross
Copy link
Collaborator Author

feross commented May 17, 2015

Nice! Thanks for publishing this so quickly :-)

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

2 participants