Simplify dom text scanner #453
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
|
✔️ No visual differences introduced by this PR. View Playwright Report (note: open the "playwright-report" artifact) |
|
If there are changes in performance characteristics, is it too bad to leave in a couple labels for the time being? Also prevents hacky ways such as https://github.com/themoeway/yomitan/blob/8e95d99e6a1891eaf9331f6384fa3b1a1065b871/ext/js/display/display.js#L1488 |
That's another situation where the inner loop should be a function. It's also not a highly traversed code path. The code that was changed in this and previous PRs was already just a single loop, albeit with an ugly body. |
djahandarie
approved these changes
Dec 29, 2023
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.
See #431 (comment):
My comment is a bit late on this, but these sub-functions should probably just be modifying the
seekTextNoteDetailsvalue that is passed into it. As it stands right now, it's repeatedly constructing and destructuring a large structure in a high-bandwidth code-path, which is probably not great. Obviously this project doesn't have performance tests currently, so I don't have hard data to back this up, but I'd imagine that this is likely less efficient.One of the reasons why this code path was originally somewhat messy was because it was a highly-used function. That's why it was doing the weird thing where it would assign all of the member fields to local variables first, and then restore them after: for (presumably) faster access in a loop. However, now that that effect is no longer being used, there is really no reason to do that, and instead the member fields should be used directly.
This is what it would look like:
https://github.com/toasted-nutbread/yomichan/commits/simplify-dom-text-scanner/
But I'm also not sure if there's a strong benefit of making that change vs just leaving the loop in the function. There's probably a lot of code locality and caching stuff that goes on in JS there, so it's hard to say what is the best way to do this in the long run without performance benchmarks.