Skip to content

Conversation

@lgoettgens
Copy link
Member

Resolves #1266.

@vtjnash could you please have a look if this is what you meant?

@codecov
Copy link

codecov bot commented Oct 10, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.71%. Comparing base (44d36ab) to head (facedc6).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1267   +/-   ##
=======================================
  Coverage   76.71%   76.71%           
=======================================
  Files          61       61           
  Lines        4887     4888    +1     
=======================================
+ Hits         3749     3750    +1     
  Misses       1138     1138           
Files with missing lines Coverage Δ
src/GAP.jl 91.86% <ø> (+0.06%) ⬆️
src/ccalls.jl 99.23% <100.00%> (ø)
src/prompt.jl 27.77% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@lgoettgens lgoettgens marked this pull request as ready for review October 12, 2025 14:07
@lgoettgens
Copy link
Member Author

The failures also happened in https://github.com/oscar-system/GAP.jl/actions/runs/18379360854, and thus seem to be independent of the changes here

@fingolfin
Copy link
Member

fingolfin commented Oct 13, 2025

To be clear: I did not switch to JuliaInterface_path being a function just because I felt like it, a lot of benchmarking preceded it

And so yeah, this PR results in worse code being generated, and a measurable drop in performance, e.g. in all calls from Julia into GAP functions. (UPDATE: at least with Julia 1.12.0 on my M1 MacBook Pro)

Before this PR:

julia> f = GAP.Globals.IsObject;

julia> @b f( (1,2,3) )
66.666 ns (3 allocs: 80 bytes)

After this PR:

julia> f = GAP.Globals.IsObject;

julia> @b f( (1,2,3) )
90.545 ns (3 allocs: 80 bytes)

That's not just a glitch, one can see the difference in the machine code.

@fingolfin
Copy link
Member

OK, maybe there is a glitch in those measurements (I am getting different numbers now -- might be a matter of P vs. E cores). I'll re-run the tests on an AMD Linux machine later. I also have a variant of this PR which just retrieves and stores the function handles explicitly (we only need a tiny handful anyway).

@fingolfin
Copy link
Member

So I did run a slightly different test on two additional machines. The test:

VERSION
using Chairmarks, BenchmarkTools, GAP
f = GAP.Globals.IsObject; t = [1,2,3];
@btime f( $t )

I did run this on an AMD Linux machine ("tutulla"), and on an M1 Mac Mini ("munk"). Results (the number in parentheses is the allocation count):

Julia master AMD this PR AMD master mac this PR mac
1.10.10 58.847 ns (3) 57.409 ns (3) 63.178 ns (3) 49.679 ns (3)
1.11.7 42.919 ns (2) 43.573 ns (2) 44.612 ns (2) 45.079 ns (2)
1.12.0 46.249 ns (2) 45.747 ns (2) 70.299 ns (2) 47.311 ns (2)
nightly error 46.593 ns (2) error 43.979 ns (2)

So it now seems this PR makes it faster ! Nice. (I got the "reverse" result on my macBook Pro once again. Does anyone know a way to test if my Julia happens to run in an E core instead of a P core at a given moment, and/or how to prevent that? sigh)

@vtjnash
Copy link

vtjnash commented Oct 14, 2025

I also have a variant of this PR which just retrieves and stores the function handles explicitly (we only need a tiny handful anyway).

FWIW, this version will be very similar to codegen (the optimizations may not even be visible to code_llvm / code_native), but will miss out on some tiny optimizations with being able to combine several branches that ccall would do with the literal call that it won't be able to do with runtime pointers. Not really measurable, since branch predictors usually do very well on benchmarks, but just a note.

@fingolfin fingolfin merged commit 5048cd5 into oscar-system:master Oct 14, 2025
467 of 476 checks passed
@lgoettgens lgoettgens deleted the lg/ji-path-nightly branch October 14, 2025 21:47
lgoettgens added a commit that referenced this pull request Oct 15, 2025
@lgoettgens lgoettgens mentioned this pull request Oct 15, 2025
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.

Nightly failures

3 participants