nsTArray_Impl::Clear (and thus ~nsTArray_Impl) have mostly-unneeded non-inline call to ShiftData

RESOLVED FIXED in Firefox 59

Status

()

Core
XPCOM
P3
normal
RESOLVED FIXED
10 months ago
a month ago

People

(Reporter: dbaron, Assigned: adrian17)

Tracking

({perf})

Trunk
mozilla59
Points:
---

Firefox Tracking Flags

(firefox57 wontfix, firefox58 wontfix, firefox59 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

10 months ago
I noticed while briefly debugging the crash in bug 1360564 (32-bit Windows) that nsTArray_Impl::Clear (inlined in ~nsTArray_Impl in that case, which means it's a very common function call) has a non-inline call to ShiftData that appears like it's mostly unnecessary.  It seems like the only valuable part is (inside ShrinkCapacity) freeing mHdr and assigning EmptyHdr() if mHdr isn't already EmptyHdr() and !UsesAutoArrayBuffer().  And I think that isn't even needed if it's the destructor rather than Clear(), because ~nsTArray_base does the same thing.

The important part of Clear seems like it's DestructRange (which is often optimized away due to lack of destructors).

I wonder whether making this codepath not go through such general code would make destruction of nsTArrays faster.

Updated

4 months ago
Priority: -- → P3
Comment hidden (mozreview-request)

Comment 2

4 months ago
mozreview-review
Comment on attachment 8919374 [details]
Bug 1361815 - Do less work in nsTArray's Clear() and destructor.

https://reviewboard.mozilla.org/r/190224/#review196034

Thank you for the patch.  I understood dbaron's comments to fix the destructor only, but thank you for going a little bit further and optimizing `Clear()` as well!

::: xpcom/ds/nsTArray.h:884
(Diff revision 1)
>      if (!base_type::IsEmpty()) {
> -      Clear();
> +      ClearAndRetainStorage();
>      }
> +    // mHdr cleanup will be handled by base destructor

OK, so the gain here is not having to go through `ShiftData` just to figure out that we need to `ShrinkCapacity` and free the data; we were going to do that anyway.

::: xpcom/ds/nsTArray.h:1738
(Diff revision 1)
> -  // A variation on the RemoveElementsAt method defined above.
> -  void Clear() { RemoveElementsAt(0, Length()); }
> +  void Clear() {
> +    ClearAndRetainStorage();
> +    Compact();
> +  }

This is basically doing the same thing as the `RemoveElementsAt` call, but now we don't need to go through `ShiftData` logic to figure out that we're just going to `ShrinkCapacity`, right?
Attachment #8919374 - Flags: review?(nfroyd) → review+
Doing a Linux-only test try to run to double-check that this is OK, and then we can land this!

Comment 4

4 months ago
hg error in cmd: hg pull gecko -r 8721270166f83c77c0e343c2380fd9e6c5a1ef41: pulling from https://reviewboard-hg.mozilla.org/gecko
abort: error: Connection timed out
(Assignee)

Comment 5

4 months ago
mozreview-review-reply
Comment on attachment 8919374 [details]
Bug 1361815 - Do less work in nsTArray's Clear() and destructor.

https://reviewboard.mozilla.org/r/190224/#review196034

> OK, so the gain here is not having to go through `ShiftData` just to figure out that we need to `ShrinkCapacity` and free the data; we were going to do that anyway.

Yeah. The main difference is that freeing of the data is now done only in `~nsTArray_base`, while previously both the destructor and `ShiftData` were checking if data needs to be freed.

The new code is a bit friendlier to the optimizer - for example, my GCC6 now correctly notices that `{ nsTArray<int> data; }` is a no-op (it couldn't do it before.).

> This is basically doing the same thing as the `RemoveElementsAt` call, but now we don't need to go through `ShiftData` logic to figure out that we're just going to `ShrinkCapacity`, right?

Yes, the end result is the same, just with a bit less unnecessary branches/indirection along the way.
The try run appears to be unhappy:

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

In particular, the Linux x64 asan runs are a source of interesting information:

https://treeherder.mozilla.org/logviewer.html#?job_id=138031273&repo=try&lineNumber=2364

I spent some time looking at this and came to the conclusion that the changes in the patch are not at fault: your changes exposed preexisting bugs in nsCookieService.  I filed those issues as bug 1410134.

I think the change to how Clear() works could land right now, which would make destruction slightly faster...but the change to the destructor itself would have to wait for other things to be fixed. :(
(Assignee)

Comment 7

3 months ago
It appears that bug 1410134 was fixed a week ago. Can the try run for this patch be retried?
(In reply to Adrian Wielgosik from comment #7)
> It appears that bug 1410134 was fixed a week ago. Can the try run for this
> patch be retried?

I pushed it on:

remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=f63b625d6603801f7824085efe4d321fecc2cab7

Thanks!
There seems to still be some cookie service crashes... I'm doing a debug build and trying to figure out what's going on, but just in case...
Flags: needinfo?(nfroyd)
Assignee: nobody → adrian.wielgosik
Depends on: 1428589
(In reply to Emilio Cobos Álvarez [:emilio] from comment #9)
> There seems to still be some cookie service crashes... I'm doing a debug
> build and trying to figure out what's going on, but just in case...

I guess you have fixed the latest round of whack-a-mole in bug 1428589?  Please re-ni? if that's not the case.
Flags: needinfo?(nfroyd)

Comment 12

a month ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd2b9748cbb6
Do less work in nsTArray's Clear() and destructor. r=froydnj

Comment 13

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dd2b9748cbb6
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
status-firefox57: affected → wontfix
status-firefox58: --- → wontfix
You need to log in before you can comment on or make changes to this bug.