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

EOF_CHAR isn't the common case, moving those first #365

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

PallHaraldsson
Copy link

No description provided.

@PallHaraldsson
Copy link
Author

I was looking into the parser, thinking if LittleDict might be faster there than Dict. Maybe not since it doesn't seem speed-critical.

I know the parser is hardly speed-critical, compared to the compiler/optimizer, or at least was. Now more code will be precompiled.

It doesn't seem to hurt to rearrange here, this likely is compiled to branches (misdirected are slow). A LittleDict of some common ASCII letters might be even faster. Note, this isn't tested or benchmarked, but I tried to be very careful to not screw up copy-pasting.

@codecov
Copy link

codecov bot commented Oct 9, 2023

Codecov Report

Merging #365 (6fe2f58) into main (a57f093) will not change coverage.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #365   +/-   ##
=======================================
  Coverage   96.56%   96.56%           
=======================================
  Files          14       14           
  Lines        4161     4161           
=======================================
  Hits         4018     4018           
  Misses        143      143           
Files Coverage Δ
src/tokenize.jl 99.07% <100.00%> (ø)

@PallHaraldsson PallHaraldsson marked this pull request as draft October 9, 2023 19:40
@PallHaraldsson
Copy link
Author

I'm a bit confused about these line:

elseif c == '−' # \minus '−' treated as hyphen '-'
        return emit(l, accept(l, '=') ? K"-=" : K"-")

or I think they must work, just unclear why not similar needed for the lines above for hyphen minus. But this must have worked...

@PallHaraldsson
Copy link
Author

Does the order matter for correctness rather than speed? That's only idea I have why my last commit worked around bug. If you trust this you could merge.

@PallHaraldsson PallHaraldsson marked this pull request as ready for review October 9, 2023 21:42
@c42f
Copy link
Member

c42f commented Oct 14, 2023

There's a very basic benchmarking script in test/benchmark.jl - does this show any improvement for before vs after this change? If you're working on performance improvements, please do prove to yourself (and everyone else :-) ) that they actually make a difference. For example, the compiler may be able to recognize very simple if-else chains and do something more efficient with them than comparing everything in order. I don't know if that happens in this case, but it could make any actual rearrangements here have no effect on the final runtime.

The order could matter if there's any overlap between categories. I think there's not, but worth checking whether of the explicit characters might be in one of the predicates.

@PallHaraldsson
Copy link
Author

At first I just meant to move the check for EOF_CHAR as it's obviously not most common. I believe the checks are in order, the compiler cann't have any idea of the true distribution. I'm doing a best guess but I haven't benchmarked.

very simple if-else chains

if some of the checks are not as simple maybe the compiler takes that into account. And I've also thought about that possibility, why I had emit at the top.

@c42f
Copy link
Member

c42f commented Oct 15, 2023

Please do benchmark it, all performance work needs benchmarking. Without that, it's easy to code changes which don't matter - needlessly churning the code or introducing complexity.

My intuition says it would be best to focus on optimizing is_identifier_start_char() - that calls into Base right now, but we should move that into JuliaSyntax and add an ascii fast path which can be inlined. In fact, we could probably split the whole block into ascii and non-ascii branches and that may well speed things up enormously - this would be my pick of the optimizations to try first here

You could also work out an approximation for the true distribution of characters if that matters - for example just compute a histogram of character frequencies.

@c42f
Copy link
Member

c42f commented Nov 1, 2023

My intuition says it would be best to focus on optimizing is_identifier_start_char

A good part of this was effectively done in #372, and it does have some measurable performance benefit.

@PallHaraldsson
Copy link
Author

PallHaraldsson commented Nov 10, 2023

no _next_token, should be optimized, have a fast path? It calls lex_identifier, that was optimized, and it isn't the main bottleneck?

is_identifier_start_char isn't most critical, did you mean is_identifier_char which is in the new fast path there.

It's interesting to see the table:
const ascii_is_identifier_char = Bool[is_identifier_char(Char(b)) for b=0x00:0x7f]

I would have thought checking for most likely (and maybe 2nd or 3rd most likely) would be faster, and possibly then a lookup table.. Lookups aren't that fast, though this one only 16 bytes. 16-entry can be made fast though, with SIMD instructions, I understand, but I'm not convinced otherwise, also bit-manipulation, not fastest?

[There are tests for BigInt literals, i.e. big macro not used, it seems, thus also no need to test for BigFloat? I'm thinking this is correct, the parser just parses a string, and hands it to the macro. I'm just thinking how much work it would be to get rid of BigFloat in Julia, it seems no problem for the parser, but BigInt can't be gotten rid of because of it.]

@KristofferC
Copy link
Member

KristofferC commented Nov 10, 2023

While some optimisation based solely on "mental work" can be useful, I find it better to work with real data from profiling and benchmarks. So if you want to work on the performance of this package (or any package for that matter) it would be a good idea to come with concrete data (in form of profiles) that show that certain functions take up a significant time and that the changes you propose improve those times (in form of benchmarks). Computers are complicated and trying to guess the effect of certain code changes is very hard. As it is right now, it feels like we are just kind of guessing which is not a way to do incremental performance improvements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants