1196461 - De-duplicate strings in the heap snapshot core dump protobuf message format

Status: RESOLVED FIXED

(DevTools :: Memory, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

It should be really easy to do, and while the size savings are probably not much (we already gzip the whole core dump) it should cut down on unnecessary mallocs and serialization/deserialization work that has to happen.

Ideally, this would happen before we build and ship any tool using heap snapshots so that we don't have to worry about backwards compatibility.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

So I have an initial version of this patch that abstracts the strings out into TwoByteString and OneByteString protobuf messages that are either the full string or a reference to a previously serialized string. It gives a decent speed up when deserializing.

However, two things:

1. This was a bit messier than I had hoped because we get so many different kinds of strings and we try to do as little converting as possible because it's expensive.

2. By abstracting the de-duplicated strings with a new protobuf message type, we force extra mallocs because a protobuf message can only be owned by another protobuf message via pointer. You can't tell the protobuf compiler to inline a message property into the owner. I'm going to profile a bit and if it seems worth it, try and work around this.

Here are my timings of 50 saves and reads of snapshots, and the size of the last one:

WITH PATCH v1:

6.4M /tmp/core-dump

[Stats samples: 50, total: 144291 ms, mean: 2885.82 ms, stddev: 28.55151638494968 ms]
[Stats samples: 50, total: 47050 ms, mean: 941 ms, stddev: 10.452088185552553 ms]

WITHOUT PATCH:

7.5M /tmp/core-dump

[Stats samples: 50, total: 153312 ms, mean: 3066.24 ms, stddev: 38.25676639402648 ms]
[Stats samples: 50, total: 67212 ms, mean: 1344.24 ms, stddev: 33.69730757952512 ms]

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

So I inlined the de-duplicated strings into their parents instead of abstracting them out as their own protobuf message type becuase of the extra mallocs that forces upon us (see comment 1). This had a pretty nice speed up in reading core dumps especially.

I couldn't remember what tabs I had open during those last benchmarks, but here are some new benchmarks:

Full Firefox runtime snapshot w/ one tab (about:blank) open

WITH PATCH v2 (de-duplicated strings inlined into parent messages):

[Stats samples: 50, total: 121159 ms, mean: 2423.18 ms, stddev: 37.14772899199636 ms]
[Stats samples: 50, total: 29932 ms, mean: 598.64 ms, stddev: 11.28695051722751 ms]
5.7M /tmp/core-dump

WITH PATCH v1 (de-duplicated strings factored out into their own protobuf message type):

[Stats samples: 50, total: 135719 ms, mean: 2714.38 ms, stddev: 20.716939610830966 ms]
[Stats samples: 50, total: 42553 ms, mean: 851.06 ms, stddev: 39.90169615569775 ms]
5.8M /tmp/core-dump

WITHOUT PATCH (no de-duplication of strings):

[Stats samples: 50, total: 136016 ms, mean: 2720.32 ms, stddev: 102.400272997866 ms]
[Stats samples: 50, total: 58244 ms, mean: 1164.88 ms, stddev: 21.5271203281115 ms]
6.7M /tmp/core-dump

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: De-duplicate strings in heap snapshot core dumps

This changeset replaces all of the

    // char16_t[]
    optional bytes someProperty = 1;

one- and two-byte string properties in the CoreDump.proto protobuf definition
file with:

    oneof {
        // char16_t[]
        bytes  someProperty    = 1;
        uint64 somePropertyRef = 2;
    }

The first time the N^th unique string is serialized, then someProperty is used
and the full string is serialized in the protobuf message. All following times
that string is serialized, somePropertyRef is used and its value is N.

Among the other things, this also changes JS::ubi::Edge::name from a raw pointer
with commented rules about who does or doesn't own and should and shouldn't free
the raw pointer to a UniquePtr that enforces those rules rather than relying on
developers reading and obeying the rules in the comments.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8652583 [details] [diff] [review]
De-duplicate strings in heap snapshot core dumps

Apologies for mixing up a bunch of stuff into the same commit, but it proved a bit difficult to pull everything apart.

Feel free to ignore the CoreDump.pb.* changes, those are generated by the protobuf compiler from the CoreDump.proto file.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8652583 [details] [diff] [review]
De-duplicate strings in heap snapshot core dumps

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

::: js/public/Utility.h
@@ +341,5 @@
>  template<typename T>
>  struct DeletePolicy
>  {
> +    void operator()(const T* ptr) {
> +        js_delete(const_cast<T*>(ptr));

Note that I don't think these const_casts are needed any more now that bug 1196378 landed. I will fix that before landing / in the next iteration of the patch.

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

FYI, the canonical type name stuff can also be safely ignored, as bug 1196634 (which is after this patch in my queue due to me not wanting to rebase too much rather than some actual dependency) removes the need to have canonical type names.

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

This try push includes this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfe372afb77d

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: De-duplicate strings in heap snapshot core dumps

This changeset replaces all of the

    // char16_t[]
    optional bytes someProperty = 1;

one- and two-byte string properties in the CoreDump.proto protobuf definition
file with:

    oneof {
        // char16_t[]
        bytes  someProperty    = 1;
        uint64 somePropertyRef = 2;
    }

The first time the N^th unique string is serialized, then someProperty is used
and the full string is serialized in the protobuf message. All following times
that string is serialized, somePropertyRef is used and its value is N.

Among the other things, this also changes JS::ubi::Edge::name from a raw pointer
with commented rules about who does or doesn't own and should and shouldn't free
the raw pointer to a UniquePtr that enforces those rules rather than relying on
developers reading and obeying the rules in the comments.

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8653431 [details] [diff] [review]
De-duplicate strings in heap snapshot core dumps

This fixes the static analysis failure and compilation errors due to missing header includes.

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5dba1ca441bb

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: De-duplicate strings in heap snapshot core dumps

This changeset replaces all of the

    // char16_t[]
    optional bytes someProperty = 1;

one- and two-byte string properties in the CoreDump.proto protobuf definition
file with:

    oneof {
        // char16_t[]
        bytes  someProperty    = 1;
        uint64 somePropertyRef = 2;
    }

The first time the N^th unique string is serialized, then someProperty is used
and the full string is serialized in the protobuf message. All following times
that string is serialized, somePropertyRef is used and its value is N.

Among the other things, this also changes JS::ubi::Edge::name from a raw pointer
with commented rules about who does or doesn't own and should and shouldn't free
the raw pointer to a UniquePtr that enforces those rules rather than relying on
developers reading and obeying the rules in the comments.

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Going to hold off on re-requesting review until I get a green try push.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=be30e8ec9c9e

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8654616 [details] [diff] [review]
De-duplicate strings in heap snapshot core dumps

Ok, try push was green.

Comment 14

[Shu-yu Guo [:shu]]

Comment on attachment 8654616 [details] [diff] [review]
De-duplicate strings in heap snapshot core dumps

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

The non-protobuf parts look okay to me. I do not have the expertise to r+ the protobuf stuff.

Just to make sure I understand, currently the unique strings set in HeapSnapshot is purely used to avoid extraneous strdup'ing the strings when deserializing?

::: toolkit/devtools/server/CoreDump.proto
@@ +77,5 @@
>          uint64 ref  = 2;
>      }
>  
>      message Data {
> +        optional uint64        id           = 1;

I don't know protobuf. What do these numbers (= 1) mean?

@@ +86,5 @@
> +        // De-duplicated two-byte string.
> +        oneof SourceOrRef {
> +            bytes  source                   = 5;
> +            uint64 sourceRef                = 6;
> +        }

Is there a way to abstract out the "oneof" boilerplate? I guess not because each variant needs to be given a unique number or something?

@@ +108,5 @@
> +    oneof TypeNameOrRef {
> +        bytes           typeName             = 2;
> +        uint64          typeNameRef          = 3;
> +    }
> +    

nit: trailing whitespace

::: toolkit/devtools/server/HeapSnapshot.cpp
@@ +387,5 @@
> +  // Should be of the form: [&] { return message.has_${stringPropertyName}ref(); }
> +  typename HasRefFunction,
> +  // Lambda of type: () -> uint64_t
> +  // Should be of the form: [&] { return message.${stringPropertyName}ref(); }
> +  typename GetRefFunction>

Can these 4 lambdas be replaced with 1 that returns a Maybe<Variant<string, uint64>>?

@@ +858,5 @@
> +    if (!stringData)
> +      return false;
> +
> +    auto buf = const_cast<char16_t*>(reinterpret_cast<const char16_t*>(stringData->data()));
> +    copyTwoByteStringToBuffer(string, RangedPtr<char16_t>(buf, length), length);

Is this really the easiest way to inflate into a std::string? Sheesh.

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8654616 [details] [diff] [review]
De-duplicate strings in heap snapshot core dumps

Jim, can you review the protobuf message related changes? (Note that the CoreDump.pb.{cc,h} files are artifacts of running the protobuf compiler on the CoreDump.proto message definition file, so you can focus your review there rather than on the generated files.)

The reason the one- and two-byte string properties are not factored out into their own abstraction/message definition is because you cannot inline child messages into their parents and it results in unnecessary mallocs that we are trying our best to avoid. See the benchmark results in earlier comments.

Comment 16

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #15)
> See the benchmark results in earlier comments.

Specifically, comment 2.

Comment 17

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Need to land this before we ship the first iteration of the memory tool because this is a backwards incompatible change.

Comment 18

[Jim Blandy :jimb]

Comment on attachment 8654616 [details] [diff] [review]
De-duplicate strings in heap snapshot core dumps

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

::: js/public/UbiNode.h
@@ +188,5 @@
> +
> +// Copy the given AtomOrTwoByteChars string into the destination buffer,
> +// inflating if necessary. Does NOT null terminate. Returns the number of
> +// characters written to destination.
> +size_t copyToBuffer(AtomOrTwoByteChars& source, RangedPtr<char16_t> destination, size_t length);

I would rather see:

class AtomOrTwoByteChars : Variant<JSAtom*, const char16_t*> {
  size_t length() const { ... }
  size_t copyToBuffer(RangedPtr<char16_t> destination, ...) { ... }
};

::: js/src/vm/UbiNode.cpp
@@ +54,5 @@
>  using JS::ubi::TracerConcreteWithCompartment;
>  
>  template<typename CharT>
>  static size_t
> +copyToBufferHelper(const CharT* src, RangedPtr<char16_t> dest, size_t length)

Suggestion, not required: I think this would read nicely as a private method of CopyToBufferMatcher.

::: toolkit/devtools/server/CoreDump.proto
@@ +44,5 @@
> +//
> +// Note that all strings are de-duplicated. The first time the N^th unique
> +// string is encountered, the full string is serialized. Subsequent times that
> +// same string is encountered, it is referenced by N. This de-duplication
> +// happens across string properties, not on a per-propery basis. For example, if

"per-property"

@@ +85,5 @@
> +
> +        // De-duplicated two-byte string.
> +        oneof SourceOrRef {
> +            bytes  source                   = 5;
> +            uint64 sourceRef                = 6;

I think the idea is that these tags are supposed to be stable. Even if our de-serialization code won't support old tags, you can get some error checking out of it. So let's try to use fresh tags for the oneof variants. I think we'll still stay below the magic maximum of 15.

Comment 19

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: De-duplicate strings in heap snapshot core dumps

This changeset replaces all of the

    // char16_t[]
    optional bytes someProperty = 1;

one- and two-byte string properties in the CoreDump.proto protobuf definition
file with:

    oneof {
        // char16_t[]
        bytes  someProperty    = 1;
        uint64 somePropertyRef = 2;
    }

The first time the N^th unique string is serialized, then someProperty is used
and the full string is serialized in the protobuf message. All following times
that string is serialized, somePropertyRef is used and its value is N.

Among the other things, this also changes JS::ubi::Edge::name from a raw pointer
with commented rules about who does or doesn't own and should and shouldn't free
the raw pointer to a UniquePtr that enforces those rules rather than relying on
developers reading and obeying the rules in the comments.

Comment 20

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Addressed both reviews' comments. Think it is a lot cleaner now, thanks both of you!

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ad12eb3a58a

Comment 21

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: De-duplicate strings in heap snapshot core dumps

This changeset replaces all of the

    // char16_t[]
    optional bytes someProperty = 1;

one- and two-byte string properties in the CoreDump.proto protobuf definition
file with:

    oneof {
        // char16_t[]
        bytes  someProperty    = 1;
        uint64 somePropertyRef = 2;
    }

The first time the N^th unique string is serialized, then someProperty is used
and the full string is serialized in the protobuf message. All following times
that string is serialized, somePropertyRef is used and its value is N.

Among the other things, this also changes JS::ubi::Edge::name from a raw pointer
with commented rules about who does or doesn't own and should and shouldn't free
the raw pointer to a UniquePtr that enforces those rules rather than relying on
developers reading and obeying the rules in the comments.

Comment 22

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Ok, this should fix teh msvc errors. Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=14c4d3795a77

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f065bda0feb4

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f065bda0feb4