1689674 - Insufficient profiler buffer size for tscroll profiles on Linux WR

Status: RESOLVED FIXED

(Testing :: Talos, defect, P3)

Description

[Markus Stange [:mstange]]

The profile_tscroll.zip profiles from this Linux x64 WR T-prof(s) job only contain about 400ms of samples. The test runs a lot longer than that. The profile buffer size needs to be increased so that the entire test duration fits in the profile.

Copy of the profile, for posterity: https://share.firefox.dev/3oBPwGA

Comment 1

[Florin Strugariu [:Bebe]]

Dave do you know who can help on this task?

Comment 2

[Florian Quèze [:florian]]

I think it's the value at https://searchfox.org/mozilla-central/rev/b2433a832c250c55255e0ee37d05192d04f20427/testing/talos/talos/test.py#962 that needs to be adjusted. It would be good to review the other gecko_profile_entries values defined in the rest of the file.

Comment 3

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Florin Strugariu [:Bebe] (needinfo me) from comment #1)

Dave do you know who can help on this task?

Kimblery, could you work with :julienw on this bug?

Comment 4

[Kimberly Sereduck :kimberlythegeek]

Kimblery, that's a new one 😆

Sure thing.

:julienw Do you have any input to add here? I can set up a call if that is more appropriate. Thanks!

Comment 5

[Julien Wajsberg [:julienw] (PTO -> Aug 24)]

hey Kimberly!

To be completely honest I'll discover the code with you :-) Florian's suggestion in comment 2 looks appropriate.

We see the current value is 1000000 which looks very small. Nowadays 1 entry equals 8 bytes, so 1M entries is 8MB. This unit "entry" is historic, and we should/will use bytes eventually (bug 1634816). Also nowadays this buffer will contain all the data for all processes, where in more ancient times we used to have one buffer of the configured size for each process.
The current defaults we use in our UI presets are 1GB, that's 128 * 1024 * 1024 entries. Is that too much here, especially in CI? Maybe we can half that to 64M entries = 512MB?

Also like Florian suggested it would be a good idea to have a quick look at other defaults in the file. I see other tests have the same default value. They also tweak the interval. We could say that with an interval 10 times larger, we can have the entry value 10 times lower. This used to be true but it's not anymore, because now we store more than just the samples, we also store all the marker information, and the amount of this information doesn't depend on the interval. So I think I'd use the same value everywhere, unless some scenario takes noticeably less/more time.

Indeed in general this all depends also on the full time a scenario will take, so if in doubt you can maybe run the tests yourselves which could give you an idea... keeping in mind that in CI / in debug builds / etc, they can run a lot slower too (so take more time, so use more memory (?)).

Note that we don't allocate this right away, rather we allocate by chunks of 1MB, so it shouldn't be a problem to use a high value. If we get OOM crashes because of that in long scenarios, this may mean that these scenarios are too long to be profiled (?). I don't know really :-)

BTW I asked a lot of questions to Gerald to be able to answer this correctly. So if you have more questions about that (and actually anything happening inside the C++ part of the profiler), he's a good person to talk to, especially that he'll be around in your afternoons, when I won't :-)

Hope this helps and isn't too confusing :-)

Comment 6

[Kimberly Sereduck :kimberlythegeek]

:julienw

That helps, thanks! I will be in touch if I have questions, or if I need to set up a call for more help :)

Comment 7

[Nazım Can Altınova [:canova]]

Attachment: Bug 1689674 - Increase the profiler entry size for talos tests r?#perftest

Previously each test had their own profiler buffer entry size, and these buffer
sizes were really low. They were either 8MB or 16MB, which wasn't really useful
for normal profiling session.

This patch increases the profiler buffer size to 128MiB for all talos tests.
It's good to keep in mind that the whole 128MiB is not allocated at the start,
this is just a hard limit for the buffer, but the profiler allocates only the
amount of memory it needs. That's why in most of the profiling sessions this
limit will not be reached.

Also the second issue was that these buffer entry limits were only set for a
subset of the tests, and not all. That's why some of the tests were using only
1MiB which is the least amount of memory profiler can allocate currently. And
this is not enough for anything. I moved this limit to general defaults
section, so all the talos tests will benefit from this.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5f1f0f5559e2
Increase the profiler entry size for talos tests r=perftest-reviewers,sparky

Comment 9

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/5f1f0f5559e2