This repository was archived by the owner on Sep 6, 2021. It is now read-only.
Repository navigation
This repository was archived by the owner on Sep 6, 2021. It is now read-only.
Apparent performance test regression in JS Quick Edit when Tern doesn't find results #3961
Description
- Run the JS Quick Edit Performance test (under the Performance tab) in sprint 24 and in sprint 25
Result: Major performance regression in sprint 25--from a few hundred ms in sprint 24 to >2 seconds each in sprint 25.
It looks like the case we're testing is one where Tern doesn't find anything, so is falling back to the old code. But it seems to take a long time to figure out that it can't find anything.
Activity
njx commentedon May 22, 2013
@jeffkenton - hmm, looks like I can't assign this directly to you. I forgot that we had this performance test when reviewing your pull request, so we didn't notice this until we were running tests at the end of the sprint. It would be good to find out why Tern is taking so long to fail on every invocation--it seems like even if it takes awhile to start up on the first time through, we should have some cached info for the later invocations.
njx commentedon Jun 3, 2013
Reviewed. Medium priority, to @jeffkenton for sprint 26.
jeffkenton commentedon Jun 3, 2013
I just tried removing my recent Tern jump-to-definition code from QuickEdit and tested this. I used the brackets.js file and searched for the function "always()", which Tern doesn't find. Without the jump-to-definition code, time taken was 3 seconds. With the jump-to-definition code it was only 100ms longer. I will look further.
njx commentedon Jun 3, 2013
Hmm. Maybe there's just something wrong with the perf test. I did try this manually and it seemed noticeably longer with Tern, but I've noticed some inconsistency in the perceived amount of time it takes to open Quick Edit with the Tern stuff so it's possible there's something else going on as well.
jeffkenton commentedon Jun 3, 2013
One thing further. Doing the same test a second time (without closing Brackets) gets results much faster in both cases -- approximately 450 ms, using the "Show Performance Data" tools.
Question: how did you disable Tern while doing this? Did you comment out the code, or do you have a trick I didn't think of?
njx commentedon Jun 5, 2013
I just went back to sprint 24 to compare, I think.
jeffkenton commentedon Jun 5, 2013
I just ran the following test with newly downloaded Brackets (on a Mac), both with Sprint 24 and Sprint 25:
The performance results are about 4 seconds for QuickEdit in both cases on my machine. I don't see what you are reporting. Note that in both cases, QuickEdit reads in about 1500 extra files to find various definitions of "always()". Jump-To-Definition doesn't find "always()".
Running the performance tests, I see identical results -- about 1.25 seconds -- for both Sprint 24 and Sprint 25.
It looks like we're not doing the same thing, somehow.
njx commentedon Jun 5, 2013
Weird. I'll spend some time today to try to verify what I thought I was seeing.
njx commentedon Jun 6, 2013
I can definitely reproduce the performance test discrepancy (note that this is not from "Show Performance Data", but from the JSQuickEdit test in the Performance tab). In sprint 24, the first JavaScript Inline Editor Creation entry is about 600ms, and all the others further down are less than 100ms. In sprint 25, all of the JavaScript Inline Editor Creation entries are over 1800ms.
However, I'm starting to suspect that this test is misleading somehow. If I try to reproduce it manually (by opening jquery.effects.core.js and putting the cursor in the
extendcall on line 272, then doing Cmd-E), I get roughly similar times in both sprint-24 and sprint-25, both perceived and by looking at "Show Performance Data"--roughly 4s. And if it is actually taking 4s to open the inline editor, then it's not clear why the test should have <100ms timings in sprint 24. So it must not be actually timing what we think it's timing.I also had to modify the test in sprint 25 to get it to run with the new code, which assumes the cursor is actually in the function name we're trying to find, whereas the old code ignored the editor contents and just looked for the specified function.
@jasonsanjose - do you know who wrote this perf test originally? Seems like we might need to figure out what's going on and try to get better timings.
jasonsanjose commentedon Jun 6, 2013
I wrote those perf tests. I haven't looked at the new tern code yet to see what needs an update though.
njx commentedon Jun 6, 2013
It seems like it must have been wrong even for the pre-tern version, though, because if I do a similar lookup manually in sprint 24 it takes 4s the first time, whereas the perf test shows ~600ms.
njx commentedon Jun 6, 2013
At this point I think we should punt this out of Sprint 26 (and maybe reassign it to @jasonsanjose?). There's enough evidence that there's no significant regression when actually using Quick Edit in the product and that this might just be an artifact of the perf test.
njx commentedon Jun 7, 2013
Moved to sprint 27.
njx commentedon Jun 7, 2013
Updated title to indicate that this is probably an issue with the perf test rather than a real regression.
jasonsanjose commentedon Jun 18, 2013
I manually tested the quick edit lookup a several times in sequence. At one point, the inline editor didn't open at all. So I tried again and noticed the recursive test error message:
Recursive tests with the same name are not supported. Timer name: INLINE_WIDGET_OPEN.My guess is that the promise from
ScopeManager.requestJumptoDef()is neither resolved nor rejected. Not sure if this could be related to the performance measurements being so far off. I'll keep digging.jasonsanjose commentedon Jun 18, 2013
The perf data is accurate, but the behavior differs between Brackets and the test 👻!
When I manually test quick edit in sequence, I fall through to the old
_findInProjectcode every time.When running the perf test, it only falls through to
_findInProjectthe first time. The remaining 4 times Tern actually picks up the definition in/Applications/Brackets Sprint 27.app/Contents/dev/src/extensions/default/JavaScriptQuickEdit/unittest-files/jquery-ui/jquery-1.7.2.js.So....a few different issues:
@jeffkenton any thoughts? To reproduce my findings during the performance test, I had to bump the timeout in
JavaScriptQuickEdit/unittests.js:544to 20000ms.JavaScriptQuickEdit/unittests.js:540(this gives us the time to open up dev tools for the next step)http://localhost:9234and open dev tools for thetestWindowtestWindowdev tools instance, set a conditional breakpoint inJavaScriptQuickEdit/main.js:119:console.log(jumpResp.fullPath),falseto print out the Tern result for each querydangoor commentedon Jul 23, 2013
@jasonsanjose I think I can answer your questions...
Granted this was a month ago and you may not remember exactly what you did, but when you manually tested did you open
extensionPath + "/unittest-files/jquery-ui"as your project, or did you try it with Brackets as your project? The code hinting has heuristics that determine where it's going to look for other files and how many files it will look at and that could make a difference between your manual tests and the performance test.Yes, the Quick Edit is for
jQuery.extendand that's pretty unambiguous.Tern probably hadn't "warmed up". It loads everything up in a background thread and my guess is that it wasn't quite ready.
Sort of. The code hinting code does remember the definitions it has found previously, so it won't need to reparse all of those files the next time around.
If there's a way for the test to know when Tern is ready and run then, that would be useful. I'm not sure what we're trying to measure here, though. When people are using JS Quick Edit, there are the two common cases:
Would our ideal be to have a test for each of these cases?
dangoor commentedon Jul 23, 2013
Removed from sprint 28. I'm thinking this should be "Low Priority" because I don't think there is a real performance regression here, rather just some inconsistent behavior with how the test interacts with the newer quick edit implementation.