Skip to content

[CLOSED] Improve TokenUtils performance through caching #8854

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
core-ai-bot opened this issue Aug 30, 2021 · 15 comments
Open

[CLOSED] Improve TokenUtils performance through caching #8854

core-ai-bot opened this issue Aug 30, 2021 · 15 comments

Comments

@core-ai-bot
Copy link
Member

Issue by MarcelGerber
Tuesday Nov 18, 2014 at 22:22 GMT
Originally opened as adobe/brackets#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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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

Done with code review.

@core-ai-bot
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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

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

@core-ai-bot
Copy link
Member Author

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
Copy link
Member Author

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

@redmunds Ready for merge.

@core-ai-bot
Copy link
Member Author

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
Copy link
Member Author

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

Merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@core-ai-bot