Skip to content

Unicode: Force-inline isgraphemebreak!#58674

Merged
Keno merged 1 commit intomasterfrom
kf/inlineisgraphemebreak
Jun 10, 2025
Merged

Unicode: Force-inline isgraphemebreak!#58674
Keno merged 1 commit intomasterfrom
kf/inlineisgraphemebreak

Conversation

@Keno
Copy link
Copy Markdown
Member

@Keno Keno commented Jun 9, 2025

When this API was added, this function inlined, which is important, because the API relies on the allocation of the Ref being elided. At some point (I went back to 1.8) this regressed. For example, it is currently responsible for substantially all non-Expr allocations in JuliaParser. Before (parsing all of Base with JuliaParser):

│     Memory estimate: 76.93 MiB, allocs estimate: 719922.

After:

│     Memory estimate: 53.31 MiB, allocs estimate: 156.

Also add a test to make sure this doesn't regress again.

@Keno Keno added backport 1.10 Change should be backported to the 1.10 release backport 1.11 Change should be backported to release-1.11 backport 1.12 Change should be backported to release-1.12 labels Jun 9, 2025
Keno added a commit to JuliaLang/JuliaSyntax.jl that referenced this pull request Jun 9, 2025
@Keno
Work around JuliaLang/julia#58674
ebdd312
@Keno Keno mentioned this pull request Jun 9, 2025
Merged
@Seelengrab
Copy link
Copy Markdown
Contributor

Seelengrab commented Jun 9, 2025

Do we know why this stopped being inlined? Is it because of the effects tainting on the ccall?

@Keno
Copy link
Copy Markdown
Member Author

Keno commented Jun 9, 2025

We must have changed the ccall cost model at some point. Usually it's not a good idea to inline, so that's sensible, it's just that it's really required in this case.

@giordano giordano added the unicode Related to unicode characters and encodings label Jun 9, 2025
@Keno
Unicdode: Force-inline isgraphemebreak!
4af681c 
When this API was added, this function inlined, which is important,
because the API relies on the allocation of the `Ref` being elided.
At some point (I went back to 1.8) this regressed. For example, it
is currently responsible for substantially all non-Expr allocations
in JuliaParser. Before (parsing all of Base with JuliaParser):
```
│     Memory estimate: 76.93 MiB, allocs estimate: 719922.
```
After:
```
│     Memory estimate: 53.31 MiB, allocs estimate: 156.
```

Also add a test to make sure this doesn't regress again.
@Keno Keno force-pushed the kf/inlineisgraphemebreak branch from b207111 to 4af681c Compare June 9, 2025 08:57
@JeffBezanson JeffBezanson added performance Must go faster parser Language parsing and surface syntax labels Jun 9, 2025
@Keno Keno changed the title Unicdode: Force-inline isgraphemebreak! Unicode: Force-inline isgraphemebreak! Jun 10, 2025
@Keno Keno merged commit d6294ba into master Jun 10, 2025
9 of 10 checks passed
@Keno Keno deleted the kf/inlineisgraphemebreak branch June 10, 2025 04:27
KristofferC pushed a commit that referenced this pull request Jun 11, 2025
@Keno
Unicode: Force-inline isgraphemebreak! (#58674)
6a96201 
When this API was added, this function inlined, which is important,
because the API relies on the allocation of the `Ref` being elided. At
some point (I went back to 1.8) this regressed. For example, it is
currently responsible for substantially all non-Expr allocations in
JuliaParser. Before (parsing all of Base with JuliaParser):
```
│     Memory estimate: 76.93 MiB, allocs estimate: 719922.
```
After:
```
│     Memory estimate: 53.31 MiB, allocs estimate: 156.
```

Also add a test to make sure this doesn't regress again.

(cherry picked from commit d6294ba)
Keno added a commit to JuliaLang/JuliaSyntax.jl that referenced this pull request Jun 12, 2025
@Keno
Work around JuliaLang/julia#58674
f3a4d05
nilesh646 pushed a commit to nilesh646/julia that referenced this pull request Jun 17, 2025
@Keno @nilesh646
Unicode: Force-inline isgraphemebreak! (JuliaLang#58674)
6c52375 
When this API was added, this function inlined, which is important,
because the API relies on the allocation of the `Ref` being elided. At
some point (I went back to 1.8) this regressed. For example, it is
currently responsible for substantially all non-Expr allocations in
JuliaParser. Before (parsing all of Base with JuliaParser):
```
│     Memory estimate: 76.93 MiB, allocs estimate: 719922.
```
After:
```
│     Memory estimate: 53.31 MiB, allocs estimate: 156.
```

Also add a test to make sure this doesn't regress again.
@KristofferC KristofferC mentioned this pull request Jun 25, 2025
Merged
60 tasks
KristofferC pushed a commit that referenced this pull request Jun 25, 2025
@Keno
Unicode: Force-inline isgraphemebreak! (#58674)
40f6d6b 
When this API was added, this function inlined, which is important,
because the API relies on the allocation of the `Ref` being elided. At
some point (I went back to 1.8) this regressed. For example, it is
currently responsible for substantially all non-Expr allocations in
JuliaParser. Before (parsing all of Base with JuliaParser):
```
│     Memory estimate: 76.93 MiB, allocs estimate: 719922.
```
After:
```
│     Memory estimate: 53.31 MiB, allocs estimate: 156.
```

Also add a test to make sure this doesn't regress again.

(cherry picked from commit d6294ba)
@KristofferC KristofferC mentioned this pull request Jun 25, 2025
Merged
71 tasks
KristofferC pushed a commit that referenced this pull request Jul 3, 2025
@Keno
Unicode: Force-inline isgraphemebreak! (#58674)
f204148 
When this API was added, this function inlined, which is important,
because the API relies on the allocation of the `Ref` being elided. At
some point (I went back to 1.8) this regressed. For example, it is
currently responsible for substantially all non-Expr allocations in
JuliaParser. Before (parsing all of Base with JuliaParser):
```
│     Memory estimate: 76.93 MiB, allocs estimate: 719922.
```
After:
```
│     Memory estimate: 53.31 MiB, allocs estimate: 156.
```

Also add a test to make sure this doesn't regress again.

(cherry picked from commit d6294ba)
@KristofferC KristofferC removed the backport 1.12 Change should be backported to release-1.12 label Jul 9, 2025
KristofferC pushed a commit that referenced this pull request Aug 19, 2025
@Keno @KristofferC
Unicode: Force-inline isgraphemebreak! (#58674)
94107d8 
When this API was added, this function inlined, which is important,
because the API relies on the allocation of the `Ref` being elided. At
some point (I went back to 1.8) this regressed. For example, it is
currently responsible for substantially all non-Expr allocations in
JuliaParser. Before (parsing all of Base with JuliaParser):
```
│     Memory estimate: 76.93 MiB, allocs estimate: 719922.
```
After:
```
│     Memory estimate: 53.31 MiB, allocs estimate: 156.
```

Also add a test to make sure this doesn't regress again.

(cherry picked from commit d6294ba)
KristofferC pushed a commit that referenced this pull request Aug 19, 2025
@Keno @KristofferC
Unicode: Force-inline isgraphemebreak! (#58674)
47ebf95 
When this API was added, this function inlined, which is important,
because the API relies on the allocation of the `Ref` being elided. At
some point (I went back to 1.8) this regressed. For example, it is
currently responsible for substantially all non-Expr allocations in
JuliaParser. Before (parsing all of Base with JuliaParser):
```
│     Memory estimate: 76.93 MiB, allocs estimate: 719922.
```
After:
```
│     Memory estimate: 53.31 MiB, allocs estimate: 156.
```

Also add a test to make sure this doesn't regress again.

(cherry picked from commit d6294ba)
@KristofferC KristofferC removed the backport 1.11 Change should be backported to release-1.11 label Aug 19, 2025
KristofferC pushed a commit that referenced this pull request Aug 19, 2025
@Keno @KristofferC
Unicode: Force-inline isgraphemebreak! (#58674)
623884c 
When this API was added, this function inlined, which is important,
because the API relies on the allocation of the `Ref` being elided. At
some point (I went back to 1.8) this regressed. For example, it is
currently responsible for substantially all non-Expr allocations in
JuliaParser. Before (parsing all of Base with JuliaParser):
```
│     Memory estimate: 76.93 MiB, allocs estimate: 719922.
```
After:
```
│     Memory estimate: 53.31 MiB, allocs estimate: 156.
```

Also add a test to make sure this doesn't regress again.

(cherry picked from commit d6294ba)
@DilumAluthge
Copy link
Copy Markdown
Member

DilumAluthge commented Aug 28, 2025 •
edited
Loading

The test is now on the 1.11.7 backports PR (47ebf95), but the test is failing (https://buildkite.com/julialang/julia-release-1-dot-11/builds/493#0198e206-5bcb-4784-9bb8-fe305e0a0962):

Unicode                                           (2) \|         failed at 2025-08-25T17:00:18.393
  | Test Failed at /cache/build/tester-amdci4-8/julialang/julia-release-1-dot-11/julia-a5f88ec176/share/julia/stdlib/v1.11/Unicode/test/runtests.jl:288
  | Expression: #= /cache/build/tester-amdci4-8/julialang/julia-release-1-dot-11/julia-a5f88ec176/share/julia/stdlib/v1.11/Unicode/test/runtests.jl:288 =#
@allocated(length(graphemes("äöüx"))) == 0
  | Evaluated: 16 == 0

@Keno Is this test expected to pass on 1.11?

@DilumAluthge
Copy link
Copy Markdown
Member

DilumAluthge commented Aug 28, 2025

This is now on the 1.11.7 backports PR (47ebf95),

Small correction.

The source code change (adding @inline) was included in the 1.11.6 backports PR (#58224, specifically 40f6d6b).

The test (@allocated(length(graphemes("äöüx"))) == 0) didn't make its way into 1.11.6, but is included in the 1.11.7 backports PR (#59336, specifically 47ebf95). And that test is now failing in CI on the 1.11.7 backports PR.

@DilumAluthge
Copy link
Copy Markdown
Member

DilumAluthge commented Aug 28, 2025

To make matters more difficult, I can't reproduce the test failure when I build backports-release-1.11 locally.

@KristofferC
Copy link
Copy Markdown
Member

KristofferC commented Aug 28, 2025

The test should just not have been backported. I don't know why it was , the 1.11 backport label is removed.

@Keno
Copy link
Copy Markdown
Member Author

Keno commented Aug 28, 2025

If it causes any trouble, just drop the test. This is a perf optimization - not critical.

DilumAluthge added a commit that referenced this pull request Aug 28, 2025
@DilumAluthge
Revert "Unicode: Force-inline isgraphemebreak! (#58674)"
9aa9ec6 
This reverts commit 47ebf95.
@DilumAluthge
Copy link
Copy Markdown
Member

DilumAluthge commented Aug 28, 2025

Cool - I pushed a revert commit (9aa9ec6) to #59336 (to revert 47ebf95, which backported the test).

Alternatively, we can rebase #59336 and drop both 47ebf95 and 9aa9ec6 - I didn't do that because I wasn't sure if a rebase+force-push would break the Backporter script.

@DilumAluthge DilumAluthge mentioned this pull request Sep 13, 2025
Merged
71 tasks
@KristofferC KristofferC mentioned this pull request Nov 20, 2025
Merged
@KristofferC KristofferC mentioned this pull request Dec 12, 2025
Merged
@KristofferC KristofferC mentioned this pull request Jan 3, 2026
Merged
DilumAluthge added a commit that referenced this pull request Jan 14, 2026
@DilumAluthge
Unicode: Remove the allocations test added in #58674
05f1383 
Cross-ref: #58674

We remove the test, but we keep the rest of the code from that PR.
DilumAluthge added a commit that referenced this pull request Jan 20, 2026
@DilumAluthge
Unicode: Remove the allocations test added in #58674
c927f6b 
Cross-ref: #58674

We remove the test, but we keep the rest of the code from that PR.
@KristofferC KristofferC removed the backport 1.10 Change should be backported to the 1.10 release label Feb 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aviatesk aviatesk aviatesk approved these changes

Assignees

No one assigned

Labels

parser Language parsing and surface syntax performance Must go faster unicode Related to unicode characters and encodings

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@Keno @Seelengrab @DilumAluthge @KristofferC @aviatesk @JeffBezanson @giordano