This is an archive of the discontinued LLVM Phabricator instance.

Increase memory of BOLT runtime instrumentation bump allocator used for writing resulting profile
ClosedPublic

Authored by Kobzol on Jun 1 2023, 7:42 AM.

Details

Summary

The BOLT instrumentation runtime uses a bump allocator that has a fixed amount of maximum memory. In some cases, this memory limit is not large enough (https://github.com/llvm/llvm-project/issues/59174). We are hitting this limit when instrumenting the Rust compiler with BOLT.

This change increases the memory of the bump allocator used for writing the resulting BOLT profile.

Diff Detail

Event Timeline

Kobzol created this revision.Jun 1 2023, 7:42 AM
Herald added a reviewer: Amir. · View Herald Transcript
Herald added a reviewer: maksfb. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Kobzol requested review of this revision.Jun 1 2023, 7:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2023, 7:42 AM

Thanks for the patch. This change is mostly OK, except that at the moment the arena size is not very simple. We actually have 3 different arenas during runtime, and by changing the default, you're changing the size of just one of them. I've highlighted in the code the different allocation arenas that we use. It would be weird to have a parameter that only changes the size of a single arena while we have two others that might overflow. What happens if we remove the calls to setMaxSize on the 2 other arenas, so all of them have the same space, the one you're parameterizing? Do we have a slowdown?

bolt/runtime/instr.cpp
196

Note: This is used to change the arena size. We have two other arenas that actually specify a custom size of 100MB using "setMaxSize", so you are overriding the size of just one of the 3 arenas.

1465

Setting hash arena to 100MB

1473–1474

Note: This is the only arena that does not call setMaxSize, so that's the one you're parameterizing. It is used to write function profile.

1565

Note: setting global arena to 100MB

1638

This is just a clone of the other arena used for writing function profile, but for apple .

Kobzol added inline comments.Jun 3 2023, 1:08 AM
bolt/runtime/instr.cpp
1473–1474

Hmm, I wonder, since the arena size situation is a bit complex, maybe a simpler solution would just be to set this specific arena to 100 MiB too?

rafauler added inline comments.Jun 5 2023, 12:50 PM
bolt/runtime/instr.cpp
1473–1474

Yes, that makes sense

Kobzol updated this revision to Diff 529226.Jun 7 2023, 3:08 AM
Kobzol retitled this revision from Allow configuring BOLT runtime instrumentation library max. allocation size to Increase memory of BOLT runtime instrumentation bump allocator used for writing resulting profile.

I removed the configuration and instead increased the size of the bump allocator that had the default size.

(I wasn't sure if I should upload a diff containing only the memory increase or if the diff should also "undo" the first diff. Hopefully I didn't mess this up).

Kobzol edited the summary of this revision. (Show Details)Jun 7 2023, 3:10 AM
Kobzol updated this revision to Diff 529238.Jun 7 2023, 3:54 AM

Looks like I did mess up and I should have uploaded the complete diff without any relative changes to previous diffs. Did that now.

Kobzol added a comment.Jun 7 2023, 3:55 AM

I checked that this change fixes our rustc issue, so the problem was indeed with this one specific bump allocator instance.

rafauler accepted this revision.Jun 7 2023, 6:06 PM

Thanks!

This revision is now accepted and ready to land.Jun 7 2023, 6:06 PM

Thank you! I'm not familiar with the landing process (and OFC I don't have merge rights) - do I have to do something else or will this eventually find its way into the main branch? :)

Amir added a comment.Jun 8 2023, 9:45 AM

@Kobzol: "Jakub Beránek <berykubik@gmail.com>" as commit author line is OK?