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

specialize on type in getindex for types #18025

Merged
merged 1 commit into from
Aug 14, 2016
Merged

Conversation

KristofferC
Copy link
Member

@KristofferC KristofferC commented Aug 14, 2016

Fixes #18021. From 3.65 -> 0.25 seconds in the benchmark code in the linked issue.

Ref #17798 for similar correction

Fix found by eavesdropping on a discussion by @vtjnash and @yuyichao :)

@KristofferC
Copy link
Member Author

Not sure we have benchmarks for this but @nanosoldier runbenchmarks(ALL, vs = ":master")

@yuyichao
Copy link
Contributor

Just discuss about something and someone else implements the fix. I hope fixing issues is always as easy as this. =)

Also ref for a similar change #18009

@KristofferC
Copy link
Member Author

It might be worth doing a search through all the .jl files for ::Type,and examine which ones are worth specializing on.

@andreasnoack
Copy link
Member

This now seems to be the general 0.5 performance trick. Has the specialization heuristics generally changed since 0.4? As I read it, this method was basically the same in 0.4.

@nanosoldier
Copy link
Collaborator

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

@yuyichao
Copy link
Contributor

The inline heuristic might have changed.

@yuyichao
Copy link
Contributor

There also seems to be some type inference/codegen issue related to Expr(:invoke), the llvm ir and asm of v32 has jl_invoke in it but the jl_invoke disappears from llvm ir after calling v32. In this particular case, jl_invoke doesn't hurt performance too much but it still cause generation of a large GC frame and boxing of all parameters passed to getindex.

@KristofferC
Copy link
Member Author

Is there an issue for that @yuyichao?

@JeffBezanson JeffBezanson merged commit 6b0bb25 into master Aug 14, 2016
@vtjnash vtjnash deleted the kc/spec_getindex_type branch August 14, 2016 16:58
@vtjnash
Copy link
Member

vtjnash commented Aug 14, 2016

This now seems to be the general 0.5 performance trick. Has the specialization heuristics generally changed since 0.4? As I read it, this method was basically the same in 0.4.

No, this fix didn't address what is broken or why, but it just addresses one of the symptoms. The actual problem is that the fast path in apply_type hasn't been optimized and is still much too slow.

tkelman pushed a commit that referenced this pull request Aug 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants