Skip to content
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.

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

scanner: compute token line/column lazily on errors #624

Merged
merged 2 commits into from
Jan 30, 2025

Conversation

bluetech
Copy link
Member

The scanner functions are hot, and the line/column location tracking is quite expensive. We only use it for errors, which don't need to be fast, because we bail if there are too many; and for warnings, which are usually not shown by default. So only keep the token start pos, and compute the line/column lazily from that. This will also allow some further improvements ahead.

bench/rulescomp

before: compiled 1000 keymaps in 1.669028s
after:  compiled 1000 keymaps in 1.550411s

bench/compose:

before: compiled 1000 compose tables in 2.145217s
after:  compiled 1000 compose tables in 2.016044s

@bluetech bluetech added this to the 1.9.0 milestone Jan 28, 2025
@wismill wismill added compose Indicates a need for improvements or additions to Compose handling compile-keymap Indicates a need for improvements or additions to keymap compilation performance labels Jan 29, 2025
@wismill
Copy link
Member

wismill commented Jan 29, 2025

Really nice! With faster position computation, we could also use it more frequently to get better logs.

This reminds me this article about Megaparsec, a quite fast Haskell parser with really good error messages. One big improvement was computing the exact position (line, column) only on demand. But it also caches it, so the computation does not start from scratch each time. I do not think this is the case here, but it could maybe improve the perf further?

@bluetech
Copy link
Member Author

Nice article, talks exactly about this. I like the caching idea, it should be easy and work well for us, will try it a bit later.

@bluetech
Copy link
Member Author

I added the caching.

@wismill
Copy link
Member

wismill commented Jan 29, 2025

Really nice! Using bench-compile-keymap --layout de --variant neo --stdev 0.3, I observed a 1.075x speedup without the cache and a 1.097x speedup with the cache. That’s a lot just for token position computation!

I would love to see some automated tests based on the stderr output. That would be test/log.c with a dummy keymap. It can be a follow-up.

@wismill wismill modified the milestones: 1.9.0, 1.8.0 Jan 29, 2025
@wismill
Copy link
Member

wismill commented Jan 29, 2025

Changing the milestone, I am sure this speedup is very much welcome!

@wismill
Copy link
Member

wismill commented Jan 30, 2025

I am going to write some tests in an independent MR.

@wismill wismill closed this Jan 30, 2025
@wismill wismill reopened this Jan 30, 2025
@wismill
Copy link
Member

wismill commented Jan 30, 2025

(did not mean to close the PR, sorry for the noise)

See #630 for the tests.

The scanner functions are hot, and the line/column location tracking is
quite expensive. We only use it for errors, which don't need to be fast,
because we bail if there are too many; and for warnings, which are
usually not shown by default. So only keep the token start pos, and
compute the line/column lazily from that. This will also allow some
further improvements ahead.

bench/rulescomp
before: compiled 1000 keymaps in 1.669028s
after:  compiled 1000 keymaps in 1.550411s

bench/compose:
before: compiled 1000 compose tables in 2.145217s
after:  compiled 1000 compose tables in 2.016044s

Signed-off-by: Ran Benita <ran@unusedvar.com>
Signed-off-by: Ran Benita <ran@unusedvar.com>
@bluetech bluetech merged commit 6e97f57 into xkbcommon:master Jan 30, 2025
5 checks passed
@bluetech bluetech deleted the scanner-lazy-loc branch February 5, 2025 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compile-keymap Indicates a need for improvements or additions to keymap compilation compose Indicates a need for improvements or additions to Compose handling performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants