Skip to content

enable inline allocation of structs with pointers #34126

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
Merged
merged 1 commit into from
Apr 27, 2020
Merged

enable inline allocation of structs with pointers #34126

merged 1 commit into from
Apr 27, 2020

Conversation

JeffBezanson
Copy link
Member

No description provided.

@JeffBezanson
Copy link
Member Author

@nanosoldier runtests(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

Something went wrong when running your job:

NanosoldierError: error when preparing/pushing to report repo: failed process: Process(`git push`, ProcessExited(128)) [128]

Unfortunately, the logs could not be uploaded.
cc @maleadt

@maleadt
Copy link
Member

maleadt commented Dec 18, 2019 •
edited
Loading

Ah, forgot to add Nanosoldier as a repository collaborator after switching to his account... I pushed the report manually: https://github.com/maleadt/BasePkgEvalReports/blob/master/pkgeval-86ae65b_vs_630a551/report.md (and it's segfault-o-clock).

@JeffBezanson JeffBezanson force-pushed the jb/enable_inline_alloc branch from 86ae65b to 54133f9 Compare December 19, 2019 18:53
@JeffBezanson JeffBezanson added the DO NOT MERGE Do not merge this PR! label Dec 19, 2019
@JeffBezanson
Copy link
Member Author

@nanosoldier runtests(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

Your test job has completed - possible new issues were detected. A full report can be found here. cc @maleadt

@vtjnash
Copy link
Member

vtjnash commented Dec 20, 2019 •
edited
Loading

Working through this list, will update as I learn more:

time limited:
HTTP v0.8.8 (test duration exceeded the time limit, but works for me)

numeric and other problems with tests as written (these should be filed as issues against their respective packages):
AIBECS v0.4.1 (doesn't work locally)
Bridge v0.10.0
FastIOBuffers v0.3.1
KissMCMC v0.2.0 # confirmed flaky
LazyArrays v0.14.10
Nabla v0.12.1
PushVectors v0.2.0
SpatialJackknife v1.0.1

@JeffBezanson JeffBezanson force-pushed the jb/enable_inline_alloc branch from 54133f9 to 23159b0 Compare December 23, 2019 18:40
@JeffBezanson
Copy link
Member Author

@nanosoldier runtests(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

Your test job has completed - possible new issues were detected. A full report can be found here. cc @maleadt

@vtjnash
Copy link
Member

vtjnash commented Dec 24, 2019

Unfortunately that didn't include #34186 or #34176, so it's not especially useful. AIBECS is still probably worth looking into, but fails when I try to run it locally. RoME and OhMyREPL might be interesting (appear to have hit a deadlock inside Pkg).

@JeffBezanson JeffBezanson added this to the 1.5 milestone Jan 15, 2020
@JeffBezanson JeffBezanson force-pushed the jb/enable_inline_alloc branch from 23159b0 to 539cbf1 Compare January 28, 2020 20:50
@KristofferC
Copy link
Member

OhMyREPL might be interesting (appear to have hit a deadlock inside Pkg).

I've seen it deadlock sometimes in PkgEval. My guess is that it is this test that hangs: https://github.com/KristofferC/OhMyREPL.jl/blob/70927549f78591e9b572714b5ed5b8e3a25f795a/test/flicker.jl#L5-L19. It is most likely unrelated to this PR though.

@vtjnash vtjnash force-pushed the jb/enable_inline_alloc branch 3 times, most recently from f029e3c to b497e44 Compare February 21, 2020 20:51
@vchuravy vchuravy mentioned this pull request Feb 23, 2020
Open
@vtjnash vtjnash mentioned this pull request Feb 25, 2020
Closed
@timholy timholy mentioned this pull request Feb 26, 2020
Open
@JeffBezanson JeffBezanson force-pushed the jb/enable_inline_alloc branch from b497e44 to b9de078 Compare March 18, 2020 17:25
@JeffBezanson
Copy link
Member Author

@nanosoldier runtests(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here. cc @maleadt

This was referenced Mar 26, 2020
Merged
Merged
@JeffBezanson
Copy link
Member Author

JeffBezanson commented Mar 27, 2020 •
edited
Loading

Found the issue in EfficientGlobalOptimization. NLopt has an array of immutable structs of two pointers (references) each (Callback_Data). It passes those objects to C for use as callback closure pointers, but they no longer have persistent addresses. That will need to be fixed by making the structs mutable.

This was referenced Mar 27, 2020
Merged
Merged
Merged
@c42f c42f mentioned this pull request Apr 17, 2020
Open
@JeffBezanson JeffBezanson force-pushed the jb/enable_inline_alloc branch from 198567a to b7347ba Compare April 19, 2020 16:34
@JeffBezanson
Copy link
Member Author

@nanosoldier runbenchmarks(ALL, vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

@vtjnash
Copy link
Member

vtjnash commented Apr 20, 2020

That's looking super. Only one I wondered about (if it's real) was:

["string", "repeat", "repeat char 2"] | 1.74 (5%) ❌ | 1.00 (1%)

@JeffBezanson JeffBezanson mentioned this pull request Apr 21, 2020
Closed
@JeffBezanson
Copy link
Member Author

It looks like repeat char 2 is a really fast benchmark (~160ns). I don't see any difference locally.

@JeffBezanson JeffBezanson reopened this Apr 21, 2020
@JeffBezanson JeffBezanson force-pushed the jb/enable_inline_alloc branch from b7347ba to 13a62dc Compare April 24, 2020 18:49
@JeffBezanson JeffBezanson merged commit 8073ddf into master Apr 27, 2020
@JeffBezanson JeffBezanson deleted the jb/enable_inline_alloc branch April 27, 2020 17:20
@quinnj
Copy link
Member

quinnj commented Apr 28, 2020

Exciting!!

mbauman added a commit that referenced this pull request Apr 28, 2020
@mbauman
Merge remote-tracking branch 'origin/master' into ajf/view-of-range
7387f65 
* origin/master: (833 commits)
  Improve typesubtract for tuples (#35600)
  Make searchsorted*/findnext/findprev return values of keytype (#32978)
  fix buggy rand(RandomDevice(), Bool) (#35590)
  remove `Ref` allocation on task switch (#35606)
  Revert "partr: fix multiqueue resorting stability" (#35589)
  exclude types with free variables from `type_morespecific` (#35555)
  fix small typo in NEWS.md (#35611)
  enable inline allocation of structs with pointers (#34126)
  SparseArrays: Speed up right-division by diagonal matrices (#35533)
  Allow hashing 1D OffsetArrays
  NEWS item for introspection macros (#35594)
  Special case empty covec-diagonal-vec product (#35557)
  [GCChecker] fix a few tests by looking through casts
  Use norm instead of abs in generic lu factorization (#34575)
  [GCChecker,NFC] run clang-format -style=llvm
  [GCChecker] fix tests and add Makefile
  Add introspection macros support for dot syntax (#35522)
  Specialize `union` for `OneTo` (#35577)
  add pop!(vector, idx, [default]) (#35513)
  bump Pkg version (#35584)
  ...
@oschulz
Copy link
Contributor

oschulz commented May 3, 2020

From what I understand, this would close #14955, @JeffBezanson ? If so, I'd update the docs of UnsafeArrays, which I guess would then be largely obsolete when using Julia v1.5. This is exciting!

@mbauman mbauman mentioned this pull request May 4, 2020
Open
1 task
@JeffBezanson
Copy link
Member Author

Yes. Would be interesting to see how performance now compares with a non-trivial use of UnsafeArrays.

vtjnash added a commit that referenced this pull request May 7, 2020
@vtjnash
threading: update codegen to use atomic annotations also
8d76838 
And add load/store alignment annotations, because LLVM now prefers that
we try to specify those explicitly, even though it's not required.

This does not yet include correct load/store behaviors for objects with
inlined references (the recent #34126 PR).
@Roger-luo Roger-luo mentioned this pull request May 7, 2020
Open
@oschulz oschulz mentioned this pull request May 29, 2020
Open
@oschulz
Copy link
Contributor

oschulz commented May 29, 2020

@JeffBezanson , it's not a very involved benchmark, but looks like the result is dramatic, in a very positive way: JuliaArrays/UnsafeArrays.jl#8

@GiggleLiu GiggleLiu mentioned this pull request Jun 8, 2020
Closed
vtjnash added a commit that referenced this pull request Jul 1, 2020
@vtjnash
threading: update codegen to use atomic annotations also
4a50693 
And add load/store alignment annotations, because LLVM now prefers that
we try to specify those explicitly, even though it's not required.

This does not yet include correct load/store behaviors for objects with
inlined references (the recent #34126 PR).
vtjnash added a commit that referenced this pull request Jul 1, 2020
@vtjnash
threading: update codegen to use atomic annotations also
2699591 
And add load/store alignment annotations, because LLVM now prefers that
we try to specify those explicitly, even though it's not required.

This does not yet include correct load/store behaviors for objects with
inlined references (the recent #34126 PR).
vtjnash added a commit that referenced this pull request Jul 7, 2020
@vtjnash
threading: update codegen to use atomic annotations also
2b510b4 
And add load/store alignment annotations, because LLVM now prefers that
we try to specify those explicitly, even though it's not required.

This does not yet include correct load/store behaviors for objects with
inlined references (the recent #34126 PR).
@thofma thofma mentioned this pull request Jul 30, 2020
Closed
simeonschaub pushed a commit to simeonschaub/julia that referenced this pull request Aug 11, 2020
@vtjnash @simeonschaub
threading: update codegen to use atomic annotations also
775bdc0 
And add load/store alignment annotations, because LLVM now prefers that
we try to specify those explicitly, even though it's not required.

This does not yet include correct load/store behaviors for objects with
inlined references (the recent JuliaLang#34126 PR).
@jd-lara jd-lara mentioned this pull request Jun 17, 2021
Open
@ChrisRackauckas ChrisRackauckas mentioned this pull request Aug 5, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
minor change Marginal behavior change acceptable for a minor release performance Must go faster
Projects
None yet
Milestone
1.5
Development

Successfully merging this pull request may close these issues.
7 participants
@JeffBezanson @nanosoldier @maleadt @vtjnash @KristofferC @quinnj @oschulz