search

brackets-archive / bracketsIssues

Archive of issues in brackets.
0 stars 0 forks source link

[CLOSED] Improve TokenUtils performance through caching #8854

Open core-ai-bot opened 3 years ago

core-ai-bot commented 3 years ago

Issue by MarcelGerber Tuesday Nov 18, 2014 at 22:22 GMT Originally opened as https://github.com/adobe/brackets/pull/9964

For #9717.

Testing

Test 1 Using this file: https://gist.github.com/MarcelGerber/5c11f1031af292044708, place your cursor in the <script> tag and press Ctrl-Space.

Test 2 Using same file, place your cursor in the <body> tag and press Cmd/Ctrl-E.

MarcelGerber included the following code: https://github.com/adobe/brackets/pull/9964/commits

core-ai-bot commented 3 years ago

Comment by MarcelGerber Saturday Dec 06, 2014 at 22:05 GMT

@redmunds@TomMalbran I managed to come up with a caching solution in TokenUtils, which seems to work out quite well. I'd appreciate your feedback!

core-ai-bot commented 3 years ago

Comment by redmunds Monday Dec 08, 2014 at 17:18 GMT

@MarcelGerber This is a great start! I verified that it fixes #9717, but it will need lots more testing.

I think the (1 sec) timeout should be replaced by listening for the Document "change" event. If there's a change to the line that's cached, then clear it.

Also, there can be multiple contexts at one time, so it might be worth having a cache for each context object. This is usually used to looking ahead to next (or back at previous) token when parsing without affecting current context, so it will usually use the same cached line, but this is something to keep in mind as a future improvement.

core-ai-bot commented 3 years ago

Comment by redmunds Monday Dec 08, 2014 at 17:31 GMT

The Editor.getModeForRange() method calls TokenUtils.getModeAt() for every token in a range, so this is another opportunity for improvement using this cache.

core-ai-bot commented 3 years ago

Comment by redmunds Monday Dec 08, 2014 at 17:35 GMT

Done with code review.

core-ai-bot commented 3 years ago

Comment by MarcelGerber Monday Dec 08, 2014 at 17:53 GMT

Ugh, didn't notice there's a JSHint error. @redmunds Could you just look for a case where multiple contexts are used at the same time? The problem with caching multiple is that it can get memory-heavy real quick. A minimized file can easily have 15,000 tokens a line, and I don't think we want have really big arrays gambling around...

Ah, and the problem with getModeAt() is that it wants precise results, which is not something we usually want to have cached.

core-ai-bot commented 3 years ago

Comment by redmunds Monday Dec 08, 2014 at 18:56 GMT

Could you just look for a case where multiple contexts are used at the same time?

Search both language/CSSUtils.js and language/HTMLUtils.js and for moveNextToken and movePrevToken you will see many cases.

The problem with caching multiple is...

As I said, this just needs to be kept in mind -- we might not need this. Seems like the worst performance is in minified files where everything is in (mostly) 1 line, so a single cache should work.

core-ai-bot commented 3 years ago

Comment by MarcelGerber Tuesday Dec 09, 2014 at 20:14 GMT

Just switched getModeAt to use, ehm, cm.getModeAt. I wonder why we hadn't used it before. It's way more performant.

About doc.change events: The problem here is that we are only passed a CM editor, so AFAIK there's no way to get the corresponding Brackets doc.

core-ai-bot commented 3 years ago

Comment by redmunds Tuesday Dec 09, 2014 at 22:14 GMT

About doc.change events: The problem here is that we are only passed a CM editor

That's only true for getModeAt(). All of the other funcs seem to have editor so use editor.document.

_.sortedIndex

The problem with _.sortedIndex is that it matches an exact end value. I think this code needs to find a token whose range contains a given pos.

UPDATE: it looks like you can pass a function to _.sortedIndex so that should work.

core-ai-bot commented 3 years ago

Comment by MarcelGerber Wednesday Dec 10, 2014 at 06:20 GMT

It's actually not a Brackets Editor object, but these are actually only CodeMirror instances - so I can't use editor.document. And yeah, I verified that _.sortedIndex returns an index even for numbers not exactly in the array.

core-ai-bot commented 3 years ago

Comment by redmunds Thursday Dec 11, 2014 at 17:08 GMT

these are actually only CodeMirror instances - so I can't use editor.document.

You can listen to the CodeMirror instance "changes" event:

        cm.on("changes", function (instance, changeList) {

        });
core-ai-bot commented 3 years ago

Comment by MarcelGerber Thursday Dec 11, 2014 at 18:25 GMT

I have implemented the (still somewhat experimental) CM changes handler.

core-ai-bot commented 3 years ago

Comment by redmunds Friday Dec 19, 2014 at 19:37 GMT

@MarcelGerber This looks good. I found another scenario where this helps and added a test case in description. Squash commits so I can merge this.

core-ai-bot commented 3 years ago

Comment by MarcelGerber Friday Dec 19, 2014 at 21:09 GMT

@redmunds Ready for merge.

core-ai-bot commented 3 years ago

Comment by redmunds Friday Dec 19, 2014 at 21:26 GMT

Travis CI is failing. It does not appear to be related to this code, but I want to make sure this passes before merging.

core-ai-bot commented 3 years ago

Comment by redmunds Friday Dec 19, 2014 at 22:58 GMT

Merging.