Closed Bug 1443587 Opened 2 years ago Closed 2 years ago

micro-optimize some Telemetry code

Categories

(Toolkit :: Telemetry, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(5 files)

I think there are other things to be done, but these are things I noticed while
reading through looking for other things.
These methods are returning nsCString wrappers for static char arrays;
we only need to return nsDependentCStrings, rather than allocating and
copying entirely new nsCString objects.
Attachment #8956546 - Flags: review?(chutten)
When constructing DynamicEventInfo, we can require the extra keys to be
passed in by rvalue reference, rather than copying the entire array.
When registering dynamic events, we also don't need to construct an
entirely new nsCString to pass to the constructor; we can simply pass
the nsACString reference we received and go from there.
Attachment #8956547 - Flags: review?(chutten)
We were copying the event, rather than taking a reference to it.  Let's
do the latter instead of the former.
Attachment #8956548 - Flags: review?(chutten)
Once we've created the array of events for a given process, there's no
need to copy it in the array for all process events.  We can Move() it
instead, which is more efficient.
Attachment #8956549 - Flags: review?(chutten)
Comment on attachment 8956546 [details] [diff] [review]
part 1 - make EventInfo string getters return dependent strings

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

Makes sense.
Attachment #8956546 - Flags: review?(chutten) → review+
Comment on attachment 8956547 [details] [diff] [review]
part 2 - improve construction of DynamicEventInfos

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

I wonder what drove the original code to create copies like that.
Attachment #8956547 - Flags: review?(chutten) → review+
Comment on attachment 8956548 [details] [diff] [review]
part 3 - don't copy ChildEventData

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

I could be wrong, but I think copy elision would probably have saved us on this one... regardless, it's good to be precise.
Attachment #8956548 - Flags: review?(chutten) → review+
(In reply to Chris H-C :chutten from comment #7)
> I could be wrong, but I think copy elision would probably have saved us on
> this one... regardless, it's good to be precise.

aEvents[i] is returning a reference, unfortunately, so there wouldn't be any copy elision taking place. =/
Comment on attachment 8956549 [details] [diff] [review]
part 4 - more efficient snapshot creation

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

LGTM
Attachment #8956549 - Flags: review?(chutten) → review+
Given the still-growing uses of events, I don't imagine you expect we'll see any shift in talos for any of these?
(In reply to Nathan Froyd [:froydnj] from comment #8)
> (In reply to Chris H-C :chutten from comment #7)
> > I could be wrong, but I think copy elision would probably have saved us on
> > this one... regardless, it's good to be precise.
> 
> aEvents[i] is returning a reference, unfortunately, so there wouldn't be any
> copy elision taking place. =/

I await the assumption of power of our new Rust overlords with eager anticipation.
(In reply to Chris H-C :chutten from comment #10)
> Given the still-growing uses of events, I don't imagine you expect we'll see
> any shift in talos for any of these?

I think it would be amazing if there were talos shifts for this.  I wouldn't get my expectations up, though.
The code for serializing an EventKey in SerializeEventsArray does
something peculiar: it creates a temporary array of nsCStrings, copies
the appropriate strings from the event info for the key, and then
converts those into JavaScript strings.  But we shouldn't need to do any
copying into the temporary array.  We can do everything directly on the
strings from the event info themselves.

It seemed best to introduce a ToJSString for this purpose, which comes
in handy in a couple other places as well.

The lambda was the best way I could think of to not repeat a bunch of code, as
well as making it clear to the reader what order we were serializing things in
and making it easy to check that both static and dynamic events were
serializing things in the right order.  Other ideas welcome.
Attachment #8956582 - Flags: review?(chutten)
Comment on attachment 8956582 [details] [diff] [review]
part 5 - more efficient string serialization

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

The string array was probably introduced to limit the scope of the dynamic events changes. The lambda does more or less the same thing, and is reasonably clear.

Alternatively, we could rework the EventInfo type hierarchy to include a common interface for serialization. We are performing the same operations on the same data, it's just the retrieval that's different. And that's just because we expose the storage on the (Dynamic)EventInfo structs instead of compartmentalizing it.

Up to you if you want to go to that length. This is fine (with the whitespace nit fixed) if you don't.

::: toolkit/components/telemetry/TelemetryEvent.cpp
@@ +584,5 @@
> +      return items.append(JS::StringValue(ToJSString(cx, category))) &&
> +             items.append(JS::StringValue(ToJSString(cx, method))) &&
> +             items.append(JS::StringValue(ToJSString(cx, object)));
> +    };
> +      

whitespace.
Attachment #8956582 - Flags: review?(chutten) → review+
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0681ad7a9915
part 1 - make EventInfo string getters return dependent strings; r=chutten
https://hg.mozilla.org/integration/mozilla-inbound/rev/edf99f34ac2c
part 2 - improve construction of DynamicEventInfos; r=chutten
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7c67940a56c
part 3 - don't copy ChildEventData; r=chutten
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbf8c7f084e1
part 4 - more efficient snapshot creation; r=chutten
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c78a8fff848
part 5 - more efficient string serialization; r=chutten
Depends on: 1450690
You need to log in before you can comment on or make changes to this bug.