1379933 - Streamline ProfileBuffer's grammar

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement)

Description

[Nicholas Nethercote [inactive]]

Bug 1379565 identified the grammar formed by the ProfileBuffer's entries. This bug will streamline the grammar a bit.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove Sample entries from the ProfileBuffer

They serve no useful purpose.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Improve ProfileBuffer's handling of labels and dynamic strings

When a sample with a label and a dynamic string is written to the
ProfileBuffer, the profiler currently joins them together (up to a max length
of 512, omitting any that exceed this) and then writes a CodeLocation entry
with an empty string followed by a sequence of EmbeddedString entries. When
parsing those entries, we allow a length up to 8192, but that limit is never
reached due to the prior limit of 512.

This patch makes the following changes.

- Removes the joining at write time. Labels and dynamic strings are now written
  separately into the ProfileBuffer, without any length limitations. (Labels
  also always take up one entry, because they only require a single pointer,
  because they are always static strings.) The joining is instead done when the
  ProfileBuffer is parsed, and the max length is 1024; any strings exceeding
  this length are truncated, rather than omitted. (This also happens to be
  outside the profier's critical section.)

- Renames CodeLocation as Label and EmbeddedString as DynamicStringFragment.
  This makes the ProfileBuffer entry names better match the names used in
  GeckoProfiler.h.

- Moves AddDynamicCodeLocation(), now called addDynamicStringEntry(), into
  ProfileBuffer.

- Adds some testing of overly-long dynamic strings to the GTest.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Start all ProfilerBufferEntry methods with an upper case letter

Also, hasKind() can be removed.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Start all ProfilerBuffer methods with an upper case letter

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Start all ProfilerBufferEntry methods with an upper case letter

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Start all ProfilerBuffer methods with an upper case letter

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Tweak ProfileBuffer arguments

This patch changes ProfileBuffer arguments from pointers to references. For
functions that modify the ProfileBuffer, it also moves the argument to the end.

Comment 8

[Markus Stange [:mstange]]

(In reply to Nicholas Nethercote [:njn] from comment #2)
> Labels and dynamic strings are now written
>   separately into the ProfileBuffer, without any length limitations. (Labels
>   also always take up one entry, because they only require a single pointer,
>   because they are always static strings.)

I'm worried about this change, specifically about the lack of length limitations for dynamic strings. If I run JavaScript where the script is not in a discrete file, but instead in a data URL, won't we copy that long URL into the buffer during sampling? I think there needs to be a cap here.
Or when we're painting a document that's a very long data URL, will we copy the long dynamic string of the nsLayoutUtils::PaintFrame pseudo stack frame into the buffer in its entirety on every sample?

Comment 9

[Markus Stange [:mstange]]

(In reply to Markus Stange [:mstange] from comment #8)
> If I run JavaScript where the script is not
> in a discrete file, but instead in a data URL, won't we copy that long URL
> into the buffer during sampling?

This was the hypothesized problem in bug 1217542, but unfortunately the page in that bug no longer reproduces the bug for me and Jeff's saved testcase is gone.

Comment 10

[Markus Stange [:mstange]]

Comment on attachment 8885202 [details] [diff] [review]
(part 2) - Improve ProfileBuffer's handling of labels and dynamic strings

Review of attachment 8885202 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good except for the lack of a cap on the length on dynamic strings when they're copied into the buffer.

Comment 11

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Improve ProfileBuffer's handling of labels and dynamic strings

Updated version:

- Reduces kMaxFrameKeyLength from 1024 to 512, to match the old limit.

- If a dynamic string is present when privacy is enabled, it's now replaced
  with "(private)".

- If a dynamic string is present and longer than kMaxFrameKeyLength, it's now
  replaced with "(too long)".
  
- The GTest is updated accordingly.

Comment 12

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4d99d47b9fa2166105b03cb7cb86c18930f6e6b
Bug 1379933 (part 1) - Remove Sample entries from the ProfileBuffer. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/00674f36f7904356b5bf2aba2779fe0cfad445cc
Bug 1379933 (part 2) - Improve ProfileBuffer's handling of labels and dynamic strings. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/158b7d6d7ea6c5f4e5e6093eb2be51da1bf590ab
Bug 1379933 (part 3) - Start all ProfilerBufferEntry methods with an upper case letter. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7e6d96eb514e1f50933cc5ff8064afb0e5830da
Bug 1379933 (part 4) - Start all ProfilerBuffer methods with an upper case letter. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3fe4adc63baf237235f439667af42cc5f9d460f9
Bug 1379933 (part 5) - Tweak ProfileBuffer arguments. r=mstange.

Comment 13

[Nicholas Nethercote [inactive]]

Comment on attachment 8885968 [details] [diff] [review]
(part 2) - Improve ProfileBuffer's handling of labels and dynamic strings

Review of attachment 8885968 [details] [diff] [review]:
-----------------------------------------------------------------

Carrying over mstange's r+ from the prior version.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d4d99d47b9fa
https://hg.mozilla.org/mozilla-central/rev/00674f36f790
https://hg.mozilla.org/mozilla-central/rev/158b7d6d7ea6
https://hg.mozilla.org/mozilla-central/rev/f7e6d96eb514
https://hg.mozilla.org/mozilla-central/rev/3fe4adc63baf