1632469 - Update rust-smallvec, use its try_reserve method

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, task)

Description

[Simon Sapin (:SimonSapin)]

… instead of hacking it from outside the crate: https://searchfox.org/mozilla-central/rev/41c3ea3ee8eab9ce7b82932257cb80b703cbba67/servo/components/fallible/lib.rs#101-163

CC https://github.com/servo/rust-smallvec/pull/214

Comment 1

[Simon Sapin (:SimonSapin)]

Attachment: Bug 1632469 - Update smallvec.

Comment 2

[Simon Sapin (:SimonSapin)]

Attachment: Bug 1632469 - Use `SmallVec::try_reserve`.

Both smallvec and hashbrown have identical CollectionAllocErr types.
Dedupcicating them or adding a From conversion between them
would require a new dependency between these two creates.

Instead, this commit adds a third copy of CollectionAllocErr
in the fallible crate, with From conversions from the other two.

Comment 3

[Simon Sapin (:SimonSapin)]

I scheduled "stylebench" jobs for these commit on try and for their parent commit on central to get a perf.html comparison. The new code is mostly equivalent, except for using realloc instead of alloc + copy_nonoverlapping + dealloc. I expected only noise in the perf difference, or perhaps minor improvement from realloc. Instead the "Show only important changes" button shows multiple red entries (which I assume mean regression) and no improvement :(

The only other difference I can think of that could be meaningful is the lack of #[inline] and #[inline(never)] attributes in https://github.com/servo/rust-smallvec/pull/214. Emilio, do you think that could explain such a perf impact?

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Hard to say, SmallVec is generic so I expect inlining for almost everything to work just fine. Adding #[inline(always)] to the non-generic error conversions that are in that PR and in the patch from this bug may be worth it, as I think otherwise we might be paying a function call if those get called outside the fallible crate. But hard to say without looking at the generated code.

We use SmallVecs infallible methods quite pervasively too, I'd look at the generated code in the selector_map (that uses the fallible version), and for the cascade / invalidation / matching / style sharing code (which uses the infallible one).

Comment 5

[Emilio Cobos Álvarez [:emilio]]

I had a hunch about reserve() getting inlined inside push() and thus blowing the size of some hot loops during style resolution.

I did some try pushes:

• https://treeherder.mozilla.org/#/jobs?repo=try&revision=bdf2d274334c51b296a8fc53fe8fb30676e1aeca (base, just central)
• https://treeherder.mozilla.org/#/jobs?repo=try&revision=d97158ba707266b74128b750a7e2ebb9fc1acb3c (your patches)
• https://treeherder.mozilla.org/#/jobs?repo=try&revision=472395874062282334adadfff93b422339fbf6ab (a patch making reserve cold from push)
• https://treeherder.mozilla.org/#/jobs?repo=try&revision=e82fe075ec1827f2c2942199aa161c56d4779412 (a patch going back to always malloc() + memmove instead of realloc()).
• https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c29c605d43a5f572b3f5b8eda3430037504a6f2 (both of the above).

Comment 6

[Emilio Cobos Álvarez [:emilio]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #5)

• https://treeherder.mozilla.org/#/jobs?repo=try&revision=472395874062282334adadfff93b422339fbf6ab (a patch making reserve cold from push)

So this one didn't help much, if so only marginally: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=bdf2d274334c51b296a8fc53fe8fb30676e1aeca&newProject=try&newRevision=472395874062282334adadfff93b422339fbf6ab&framework=10

• https://treeherder.mozilla.org/#/jobs?repo=try&revision=e82fe075ec1827f2c2942199aa161c56d4779412 (a patch going back to always malloc() + memmove instead of realloc()).

This one looks busted because I had the cargo patch in the other patch...

• https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c29c605d43a5f572b3f5b8eda3430037504a6f2 (both of the above).

But with this one perf looks good again: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=bdf2d274334c51b296a8fc53fe8fb30676e1aeca&newProject=try&newRevision=1c29c605d43a5f572b3f5b8eda3430037504a6f2&framework=10

So it seems like realloc() is slower than malloc() + memmove at least from stylo, from multiple threads. Mike, is that expected? May it be related to the jemalloc arenas thing that we use from the style system or such?

Comment 7

[Emilio Cobos Álvarez [:emilio]]

I tried to profile this but I couldn't get symbols for the stuff under replace_realloc, so not all that useful.

Comment 8

[Emilio Cobos Álvarez [:emilio]]

https://perfht.ml/3aBnPqe is the uri just in case.

Comment 9

[Mike Hommey [:glandium]]

Try comparing when PHC is disabled?

Comment 10

[Emilio Cobos Álvarez [:emilio]]

Base: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b17eb313e9e2714cc123e0682a51a58345631fc
Patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d737b3965ebbeec389d75c56f0692ab76648369f
Base minus PHC: https://treeherder.mozilla.org/#/jobs?repo=try&revision=df5fb9b371a3bb91dab8149c4ecc07918da71f34
Patch minus PHC: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a0bd67e8593257d2a60c2b6e862ca9ce77ed376

Comment 11

[Emilio Cobos Álvarez [:emilio]]

It doesn't seem like PHC significantly affects perf here: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=6b17eb313e9e2714cc123e0682a51a58345631fc&newProject=try&newRevision=4a0bd67e8593257d2a60c2b6e862ca9ce77ed376&framework=10

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:SimonSapin, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 13

[Simon Sapin (:SimonSapin)]

These patches (specifically using realloc instead of malloc + copy + free) cause a regression that still needs to be investigated / fixed, but I don’t know when I’ll get to this. If it helps triage I don’t mind marking this bug "resolved incomplete" or something similar, in the meantime.

Comment 14

[Emilio Cobos Álvarez [:emilio]]

Bug 1681003 was the allocator bug.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8ba485d9c77b
Update smallvec. r=manishearth

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f4ee87577e77
Use `SmallVec::try_reserve`. r=manishearth

Comment 17

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/8ba485d9c77b

Comment 18

[Narcis Beleuzu [:NarcisB]]

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