various edge-case loading optimizations: #709
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
ikreymer
commented
Oct 27, 2024
ikreymer
commented
- if too many range requests for same URL are being made, try skipping/failing right away to reduce load
- assume main browser context is used not just for service workers, always enable
- check false positive 'net-aborted' error that may actually be ok for media, as well as documents
- improve logging
- possible fix for issues in Browser disconnected (crashed?) #706
- interrupt any pending requests (that may be loading via browser context) after page timeout, log dropped requests
|
I'm wondering if we should also move the |
tw4l
approved these changes
Oct 28, 2024
There was a problem hiding this comment.
Looks good, a few small comments. I tested against a few of the examples in #706 (comment) and am not seeing any browser crashes, though these were admittedly smaller-scoped crawls.
|
This is ready for re-review - though probably want to merge test fixes from #710 first |
|
Main changes are actually streaming 206 responses (!) which were before always loaded into memory, also failing duplicate 206 responses that are not from 0- to avoid additional load. |
- if too many range requests for same URL are being made, try skipping/failing right away to reduce load - assume main browser context is used not just for service workers, always enable - check false positive 'net-aborted' error that may actually be ok for media, as well as documents - improve logging - possible fix for issues in #706 - interrupt any pending requests (that may be loading via browser context) after page timeout, log dropped requests
…amed by only checking 200, not 206 (ack!) status code new logic: - if content length > 25MB (text rewrite limit), always stream, won't do any rewriting (should be fairly rare) - if content length > 5MB, always stream, unless essential resource (eg. html/js/css) which requires in-memory fetch for possible rewriting - if content length is unknown, stream if non-error code <300, as error codes likely aren't very large.
- add fetchContinued to avoid double-handling requestPaused if intercepted both in browser context and page context - rename isServiceWorker -> isBrowserContext, write record in fetch response if no page context - don't write 'no payload' requests in browser context, as they may be redirects reusing same requestId, and 204 will get skipped anyway - don't auto-attempt aborted media, already handled via behavior fetch
- interrupt pending requests when page is finished, so pageinfo record is written after - add pageFinished flag to recorder, remove unused 'skipping' flag - renable attempt refetch, should be using dedup
tw4l
force-pushed
the
range-load-optimizations
branch
from
5cacea5 to
5ea30d3
Compare
tw4l
approved these changes
Oct 31, 2024
Co-authored-by: Tessa Walsh <[email protected]>
tw4l
reviewed
Oct 31, 2024
ikreymer
added a commit
that referenced
this pull request
Nov 13, 2024
- fix: prefer streaming current response via takeStream, not only when size is unknown - ensure partial range requests are not async fetched, only full responses - don't serialize zero-payload responses - don't serialize 206 responses if there is size mismatch
ikreymer
added a commit
that referenced
this pull request
Nov 14, 2024
various fixes for streaming, especially related to range requests - follow up to #709 - fix: prefer streaming current response via takeStream, not only when size is unknown - don't serialize async responses prematurely - don't serialize 206 responses if there is size mismatch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.