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

simplify @allocated macro #33717

Merged
merged 3 commits into from
Nov 15, 2019
Merged

simplify @allocated macro #33717

merged 3 commits into from
Nov 15, 2019

Conversation

vtjnash
Copy link
Sponsor Member

@vtjnash vtjnash commented Oct 29, 2019

There were a long list of gotchas in the @allocated macro to deal with problems that it couldn't actually solve. The simplified macro doesn't have those issues—and even solves some of the issues it was trying to address. This needed an adjustment to @testset to avoid it triggering the compiler heuristic that disables inference.

while false; end # compiler heuristic: compile this block (alter this if the heuristic changes)
local b0 = Ref{Int64}(0)
local b1 = Ref{Int64}(0)
gc_bytes(b0)
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this code is compiled, shouldn't it be ok for gc_bytes() to return a value?

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some code paths that'll compile but not infer this code. But anyways, it seemed nicer to be at least robust against ever including @allocated in the overhead.

Changes testset to avoid compiler heuristics (copyast) that disables inference.
And changes the allocated macro to rely less on inference to elid allocations for the machinary itself.
@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Nov 14, 2019

Just to make sure this doesn't affect nanosoldier: @nanosoldier runbenchmarks("shootout", vs=":master")

@nanosoldier
Copy link
Collaborator

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

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.

None yet

3 participants