Add bailout case to sourcemap searching code #25212
Conversation
src/services/services.ts
Outdated
| if (comment) { | ||
| return comment[1]; | ||
| } | ||
| // If we see a nonwhitespace line, break, to avoid scanning up the entire file | ||
| else if (!lineText.match(whitespaceRegExp)) { |
There was a problem hiding this comment.
The only other comment allowed after //# sourceMappingURL is //# sourceURL, which also needs to be skipped.
|
I'm questioning the value of checking in the massive test file. It's not more likely to fail than a small test file. If performance regresses, it'll just take longer to pass. Which is not a situation that anyone's likely to notice. Other than that -- I've confirmed these changes fix #25209 and they look fine to me. |
20s longer of tests usually get noticed. Usually. At least they have in the past, since the test duration metrics are recorded locally, and that duration is amplified when testing on less powerful machines. (And if anything the git history will be useful if it regresses and someone notices only by how their tests have started to take significantly longer) Unfortunately, I don't think I can actually make it large enough to crash or otherwise error (without crashing both before/after for other reasons anyway, like the file being way too large to fit in memory), and we don't have any test logic for fail-on-too-slow (which is always hard to set a bar for when test are running on multiple machines with different performance profiles). There is a test timeout of 40s... I could try to make it large enough to trigger the timeout in a pre-fixed situation? It probably just means doubling the length of the file to 1m lines. |
|
Usually performance tests are separate from functional tests, because of the reasons you pointed out. These two kinds of tests have fundamentally different definitions of pass and fail. I'm not too familiar with how we structure tests in this repo. If there's precedence for this, and you believe it adds value, then OK by me. Thanks for the fix! |
Fixes #25209
The test I added (a namespace with 580k blank lines inside it) drops from 21s to 1s with this change, so I think this aughta solve the problem.
cc @rbuckton who's more familiar with the performance aspects of v8's implementation, since optimizing this, even though it's only run once per file (per edit, potentially, since files are discarded on edit), is probably a good idea. It's be nice if you could remark on this style of matching - I don't think string slicing is worse than the
gmregex we were using before, but I'm unsure, personally (I think it's better with the bail case we have now?).