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

Mark hot functions in the runtime. #1501

Closed
wants to merge 1 commit into from

Conversation

evincarofautumn
Copy link
Contributor

This attribute is supposed to improve code locality and branch prediction for hot functions. It can be inferred by profile-guided optimisation, but I think it makes sense to encode in the source directly. All but mono_jit_runtime_invoke are in SGen.

I mainly wanted to solicit feedback about whether this seems worthwhile—running the benchmark suite, it improves performance on more complex workloads (lcscbench, raytracer3, sharpchess) but worsens microbenchmarks (onelist, lists).

@monoadmin
Copy link

Hello! I'm the build bot for the Mono project. I need approval from a Mono team member to build this pull request. A team member should reply with "approve" to approve a build of this pull request, "whitelist" to whitelist this and all future pull requests from this contributor, or "build" to explicitly request a build, even if one has already been done. Contributors can ignore this message.

@alexrp
Copy link
Contributor

alexrp commented Jan 13, 2015

I'd be interested to know exactly what effect this has on generated code.

@kumpera
Copy link
Contributor

kumpera commented Jan 13, 2015

What's the binary size impact of enabling this?

@kumpera
Copy link
Contributor

kumpera commented Jan 13, 2015

Another thing, if a pgo build trained on the complex benchmarks gets the same results, it's worth considering augmenting the build system instead to do it as we would not have to revisit those decisions
6/12 months down the line.

Alex at one point explored us moving mono to -O3. That, pgo and lto could get us significant gains for very little coding effort.

@migueldeicaza
Copy link
Contributor

Does anyone have objections to the patch as is?

@migueldeicaza
Copy link
Contributor

approve

@alexrp
Copy link
Contributor

alexrp commented Jan 24, 2015

I have no objections per se, but I'm wary of hitting merge before we have some numbers. These attributes can have significant impact on inlining and thus perf/size.

@migueldeicaza
Copy link
Contributor

Alex, ok, can you get the numbers for us?

@alexrp
Copy link
Contributor

alexrp commented Jan 26, 2015

Size on x86-64 before this patch:

-rwxrwxr-x 1 alexrp alexrp 15109169 jan 26 08:03 mono/mini/mono-sgen

With this patch:

-rwxrwxr-x 1 alexrp alexrp 15109969 jan 26 08:12 mono/mini/mono-sgen

So I'd say a negligible difference there.

As for perf, I wouldn't even know where to begin benchmarking these areas of SGen sensibly. I'd rather defer to the performance team here.

@migueldeicaza
Copy link
Contributor

@evincarofautumn Jon, could you please bring up the numbers on the mailing list so we can discuss there the pros/cons? And you can show us what the speedup is like on the good cases and what the slowdown is on the bad ones.

@lewurm
Copy link
Contributor

lewurm commented Mar 28, 2016

@evincarofautumn is there still interest to get this merged? it would be nice if you could rebase it and benchmark it, since we have the infrastructure now :)

@evincarofautumn
Copy link
Contributor Author

benchmark

@akoeplinger
Copy link
Member

build

@akoeplinger
Copy link
Member

builddeb

@dnfclas
Copy link

dnfclas commented Nov 3, 2016

@evincarofautumn, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

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

9 participants