<a class="header-button" href="https://bugzilla.mozilla.org/home" title="Go to home page"> Bugzilla

Assignee

Comment 1

•

5 years ago

Investigated how to have the compiler improve codegen. Had some ideas that resulting in https://github.com/djg/unsafe-poke.

:jrmuizel suggested https://github.com/devashishdxt/desse.

Going to experiment with both.

Comment 2

•

5 years ago

Another (~minor) flaw with bincode is that it's too dynamic to understand that enum tags can fit within a u8, so it serializes them all to u32s (exception: it natively understands Option).

Also note that all of our metrics around DLs are a little broken, as things got refactored to the point where they don't properly measure things. For instance "DL consume" wraps a region that was previously a sync call, but is now async, so it measures nothing.

I also half-developed a webrender_api/display_list_stats feature which you can enable via Cargo, which will give stats about the size and quantities of different display items, but it's not properly hooked up to aggregate stats for all the DLs in a scene, so it kinda just pukes a bunch of numbers which are only useful for feeling out big picture details.

In the DL refactor I also removed an optimization that you could consider re-introducing: a duplicate of DisplayItem that takes certain fields by-reference which bincode will serialize the same as if they were inline, but helps us avoid copying everything into a fully materialized enum. Serializing the CommonItemProperties and some bulkier matrices by-reference could potentially be a nice savings.

More generally since ~75% of our display list size is TextDisplayItem and its Glyphs array, just trying to tune that would be most fruitful.

Comment 3

•

5 years ago

Oh also in case you weren't aware:

I created/implemented this RFC to make enum deserialization more effecient: https://github.com/rust-lang/rfcs/blob/master/text/2195-really-tagged-unions.md

I then forked serde to add support for this in a specific case for deserialize_in_place: https://github.com/servo/serde/commit/84b2795d2a7b5312125a99b1ef11c67fd8d17c35

Specifically, I require that one of the variants of the enum has no payload ("None-like"), so that I can (re)-initialize the tag of the enum to this value for exception-safety while we prod at the union.

(Otherwise you're forced to construct the enum's payload and then copy it into the enum, as rust doesn't have placement new)

Assignee

Comment 4

•

5 years ago

Thanks for the extra info :Gankro. I've been messing with a combination of the ideas in desse and bincode_max_size that :jrmuizel wrote and combined them into https://github.com/djg/peek-poke. Rust appears to be able optimize all the max_size() calls when they are forced inline and the code generated is better than bincode:

Benchmarking struct::serialize/peek_poke::poke_into: Collecting 100 samples in e                                                                                struct::serialize/peek_poke::poke_into
                        time:   [2.7892 ns 2.7939 ns 2.7992 ns]
Found 5 outliers among 100 measurements (5.00%)
  3 (3.00%) low mild
  1 (1.00%) high mild
  1 (1.00%) high severe
Benchmarking struct::serialize/bincode::serialize: Collecting 100 samples in est                                                                                struct::serialize/bincode::serialize
                        time:   [31.064 ns 31.133 ns 31.205 ns]
Found 7 outliers among 100 measurements (7.00%)
  1 (1.00%) low mild
  4 (4.00%) high mild
  2 (2.00%) high severe

Benchmarking struct::deserialize/peek_poke::peek_from: Collecting 100 samples in                                                                                struct::deserialize/peek_poke::peek_from
                        time:   [1.4875 ns 1.4910 ns 1.4949 ns]
Found 9 outliers among 100 measurements (9.00%)
  2 (2.00%) low severe
  1 (1.00%) low mild
  5 (5.00%) high mild
  1 (1.00%) high severe
Benchmarking struct::deserialize/bincode::deserialize: Collecting 100 samples in                                                                                struct::deserialize/bincode::deserialize
                        time:   [21.277 ns 21.314 ns 21.357 ns]
Found 7 outliers among 100 measurements (7.00%)
  1 (1.00%) low severe
  4 (4.00%) high mild
  2 (2.00%) high severe

The downside with this current approach is that it doesn't know how large the serialized data is, only the maximum size, so could be quite wasteful. The unsafe-poke approach generated bincode compatible serialized format (because I didn't want to implement deserialization at the time) but does know the exact size written into memory. VTune on Windows showed a 5x improvement for dl-mutate with text.

Assignee

Comment 5

•

5 years ago

NI? :Gankro to get feed back on the direction that should be taken here:

Do we want compact serialization with no "holes" in the written output?
To be serde and bincode compatible?

Flags: needinfo?(a.beingessner)

Comment 6

•

5 years ago

Using max-size really shouldn't be very wasteful, if at all. Because it's only conservative per-element, we don't "throw away" the extra slack space we request (it can be used for the next one). Also we already pre-reserve a fairly large buffer (100kb iirc?).

I expect it will still be fairly important for the serialization to have memory-compressing effects. The worst-case size of a display item is quite large. For instance, there are several Option<Matrix4d>s in there, and it's really nice to just read/write a single byte for those in the None case.

I don't think being specifically bincode compatible is important. I used bincode because I wanted to leverage the advantages of the wider ecosystem (lots of eyes for safety, potential for people to improve it for us).

It's notable that bincode is intended as a long-term data storage/interchange format, and so it makes several concessions to portability. Off the top of my head, usizes become u64s and enums become u32s (all of our enums are marked repr(u8)).

There has historically been discussion of whether supporting variable-length encoding of integers is worthwhile for bincode. But if we drop the portability constraint (our format only needs to be able to pass data between processes from the same build, on the same machine), the value of variable-length encoding isn't that great (although maybe the compression wins would be dramatic on our inputs, who knows!)

I would also note that your deserialize benchmark is a bit unfair -- you should be using the deserialize_in_place method to compare against our state of the art (it's hidden because it only exists for me to use in webrender :p). You also will need to request the deserialize_in_place feature for serde_derive. (Removing the deserialize_in_place feature, and the derives it produces for every single derive(Deserialize) type in firefox would honestly be a nice benefit of moving to peek-poke though).

You might also want to beef up your benchmark a bit -- display items like TextDisplayItem and ReferenceFrameDisplayItem are >100 bytes with lots of nested enums. Also they are always deserialized into an enum (DisplayItem itself). A big issue we ran into with bincode::serialize was that the code for DisplayItem was so complex that it couldn't be profitably inlined/specialized, and this was really unfortunate given that almost every path statically picks one of the DisplayItem cases.

Also it's worth noting that the array of GlyphInstance that follows TextDisplayItem is a huge percentage of display lists, so if bincode and your solution are competitive there, wins elsewhere may get completely washed out.

Also to be clear we do not deserialize the GlyphInstance array (or any other array) into a Vec, we produce an iterator which deserializes the items on demand (not in-place). I didn't want to allocate any arrays that might just get thrown away. Although the GlyphInstance array in particular is currently collected into a Vec to use as a key before even checking if it's in the interner, which is a bug that should be fixed.

Flags: needinfo?(a.beingessner)

Assignee

Comment 7

•

5 years ago

Some more serialization experiments:

At opt-level = 2 the following code is generated for sample webrender_api::CommonItemProperties:

foo::test:
 push    rbp
 mov     rbp, rsp
 mov     eax, dword, ptr, [rsi]
 mov     dword, ptr, [rdi], eax
 mov     eax, dword, ptr, [rsi, +, 4]
 mov     dword, ptr, [rdi, +, 4], eax
 mov     eax, dword, ptr, [rsi, +, 8]
 mov     dword, ptr, [rdi, +, 8], eax
 mov     eax, dword, ptr, [rsi, +, 12]
 mov     dword, ptr, [rdi, +, 12], eax
 mov     rax, qword, ptr, [rsi, +, 16]
 mov     qword, ptr, [rdi, +, 16], rax
 mov     eax, dword, ptr, [rsi, +, 24]
 mov     dword, ptr, [rdi, +, 24], eax
 mov     eax, dword, ptr, [rsi, +, 28]
 mov     dword, ptr, [rdi, +, 28], eax
 mov     eax, dword, ptr, [rsi, +, 32]
 mov     byte, ptr, [rdi, +, 32], al
 cmp     eax, 1
 jne     LBB7_1
 mov     rax, qword, ptr, [rsi, +, 40]
 mov     qword, ptr, [rdi, +, 33], rax
 mov     eax, dword, ptr, [rsi, +, 48]
 mov     dword, ptr, [rdi, +, 41], eax
 lea     rax, [rsi, +, 52]
 jmp     LBB7_3
LBB7_1:
 mov     rax, qword, ptr, [rsi, +, 48]
 mov     qword, ptr, [rdi, +, 33], rax
 mov     eax, dword, ptr, [rsi, +, 36]
 mov     dword, ptr, [rdi, +, 41], eax
 lea     rax, [rsi, +, 40]
LBB7_3:
 mov     eax, dword, ptr, [rax]
 mov     dword, ptr, [rdi, +, 45], eax
 cmp     qword, ptr, [rsi, +, 56], 1
 jne     LBB7_4
 mov     byte, ptr, [rdi, +, 49], 1
 mov     rax, qword, ptr, [rsi, +, 64]
 mov     qword, ptr, [rdi, +, 50], rax
 movzx   eax, word, ptr, [rsi, +, 72]
 mov     word, ptr, [rdi, +, 58], ax
 mov     eax, 60
 jmp     LBB7_6
LBB7_4:
 mov     byte, ptr, [rdi, +, 49], 0
 mov     eax, 50
LBB7_6:
 mov     cl, byte, ptr, [rsi, +, 80]
 mov     byte, ptr, [rdi, +, rax], cl
 lea     rax, [rdi, +, rax, +, 1]
 pop     rbp
 ret

Marking all the structs and tuples as #[repr(C)] improves codegen:

foo::test:
 push    rbp
 mov     rbp, rsp
 mov     eax, dword, ptr, [rsi]
 mov     dword, ptr, [rdi], eax
 mov     eax, dword, ptr, [rsi, +, 4]
 mov     dword, ptr, [rdi, +, 4], eax
 mov     eax, dword, ptr, [rsi, +, 8]
 mov     dword, ptr, [rdi, +, 8], eax
 mov     eax, dword, ptr, [rsi, +, 12]
 mov     dword, ptr, [rdi, +, 12], eax
 mov     rax, qword, ptr, [rsi, +, 16]
 mov     qword, ptr, [rdi, +, 16], rax
 mov     eax, dword, ptr, [rsi, +, 24]
 mov     dword, ptr, [rdi, +, 24], eax
 mov     eax, dword, ptr, [rsi, +, 28]
 mov     dword, ptr, [rdi, +, 28], eax
 mov     al, byte, ptr, [rsi, +, 32]
 mov     byte, ptr, [rdi, +, 32], al
 mov     rax, qword, ptr, [rsi, +, 40]
 mov     qword, ptr, [rdi, +, 33], rax
 mov     eax, dword, ptr, [rsi, +, 48]
 mov     dword, ptr, [rdi, +, 41], eax
 mov     eax, dword, ptr, [rsi, +, 52]
 mov     dword, ptr, [rdi, +, 45], eax
 cmp     qword, ptr, [rsi, +, 56], 1
 jne     LBB8_1
 mov     byte, ptr, [rdi, +, 49], 1
 mov     rax, qword, ptr, [rsi, +, 64]
 mov     qword, ptr, [rdi, +, 50], rax
 movzx   eax, word, ptr, [rsi, +, 72]
 mov     word, ptr, [rdi, +, 58], ax
 mov     eax, 60
 jmp     LBB8_3
LBB8_1:
 mov     byte, ptr, [rdi, +, 49], 0
 mov     eax, 50
LBB8_3:
 mov     cl, byte, ptr, [rsi, +, 80]
 mov     byte, ptr, [rdi, +, rax], cl
 lea     rax, [rdi, +, rax, +, 1]
 pop     rbp
 ret

At opt-level = 3 the following code is generated:

foo::test:
 push    rbp
 mov     rbp, rsp
 movups  xmm0, xmmword, ptr, [rsi]
 movups  xmmword, ptr, [rdi], xmm0
 mov     rax, qword, ptr, [rsi, +, 16]
 mov     qword, ptr, [rdi, +, 16], rax
 mov     rax, qword, ptr, [rsi, +, 24]
 mov     qword, ptr, [rdi, +, 24], rax
 mov     al, byte, ptr, [rsi, +, 32]
 mov     byte, ptr, [rdi, +, 32], al
 mov     rax, qword, ptr, [rsi, +, 40]
 mov     qword, ptr, [rdi, +, 33], rax
 mov     rax, qword, ptr, [rsi, +, 48]
 mov     qword, ptr, [rdi, +, 41], rax
 cmp     qword, ptr, [rsi, +, 56], 1
 jne     LBB8_1
 mov     byte, ptr, [rdi, +, 49], 1
 mov     rax, qword, ptr, [rsi, +, 64]
 movzx   ecx, word, ptr, [rsi, +, 72]
 mov     qword, ptr, [rdi, +, 50], rax
 mov     word, ptr, [rdi, +, 58], cx
 mov     eax, 60
 jmp     LBB8_3
LBB8_1:
 mov     byte, ptr, [rdi, +, 49], 0
 mov     eax, 50
LBB8_3:
 mov     cl, byte, ptr, [rsi, +, 80]
 mov     byte, ptr, [rdi, +, rax], cl
 lea     rax, [rdi, +, rax, +, 1]
 pop     rbp
 ret

Assignee

Comment 8

•

5 years ago

(In reply to Alexis Beingessner [:Gankro] from comment #6)

(Removing the deserialize_in_place feature, and the derives it produces for every single derive(Deserialize) type in firefox would honestly be a nice benefit of moving to peek-poke though).

:jrmuizel said the same thing in #gfx, which convinced me to keep investigating.

Assignee

Comment 9

•

5 years ago

Attached file Bug 1550640 - P1: Refactor ItemRange to contain u8 slices for byte ranges. — Details

This refactor is in preparation for P3.

When refactoring next_raw() to use peek_from instead of
deserialize_in_place, it became clear that ItemRange is holding a slice of
bytes from the incoming serialized display list and peek_from could be adapted
work directly on the byte slice.

It was also noticed that the get() interface was potentially unsafe; any
ItemRange can be passed into get() for any display list.

Assignee

Comment 10

•

5 years ago

Attached file Bug 1550640 - P2: Remove dead code resulting from refactoring of ItemRange. — Details

Having ItemRange contain a byte slice means that the display_list and
pipeline_id don't need to be threaded through code that accesses items from
ItemRange.

Assignee

Comment 11

•

5 years ago

Attached file Bug 1550640 - P3: Replace bincode with peek-poke. — Details

Replace serde-derived bincode with custom binary
serialization/deserialization that generates more efficient code at rustc
opt-level = 2.

Assignee

Comment 12

•

5 years ago

Attached file Bug 1550640 - P4: Remove bincode. — Details

Assignee

Comment 13

•

5 years ago

Attached file Bug 1550640 - P5: Vendor rust crates peek-poke, peek-poke-derive. (obsolete) — Details

Comment 14

•

5 years ago

Can you share any full-integration performance benefits you got from this change?

Comment 15

•

5 years ago

Reviewing peek-poke is going to take me a while.

A possible nice thing to do for yourself would be to land P1 and P2 now since they're just nice refactors?

Bogdan Tara[:bogdan_tara | bogdant]

Comment 16

•

5 years ago

Pushed by dglastonbury@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/55e8d218af69 P1: Refactor ItemRange to contain u8 slices for byte ranges. r=Gankro https://hg.mozilla.org/integration/autoland/rev/8380f5f60101 P2: Remove dead code resulting from refactoring of ItemRange. r=Gankro

Comment 17

•

5 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/55e8d218af69
https://hg.mozilla.org/mozilla-central/rev/8380f5f60101

Status: ASSIGNED → RESOLVED

Closed: 5 years ago

status-firefox69: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla69

Mayank Bansal

Comment 18

•

5 years ago

only two parts have landed

Status: RESOLVED → REOPENED

Flags: needinfo?(btara)

Resolution: FIXED → ---

Bogdan Tara[:bogdan_tara | bogdant]

Comment 19

•

5 years ago

The bug was marked with bugherder and automatically closed.
If you don't want the bug to be automatically closed, please use the "leave-open" keyowrd in the tracking section.

Flags: needinfo?(btara)

BugBot [:suhaib / :marco/ :calixte]

Updated

•

5 years ago

status-firefox69: fixed → affected

Assignee

Comment 20

•

5 years ago

•

Edited

From VTune on Windows, testing with Miko's version of dl-mutate test I would see Content Process as the number one Thread with the following kind of relationship:

Content Process (TID: 10528)

bincode::internal::serialize_into<mut webrender_api::display_list::UnsafeVecWriter*, webrender_api::display_item::DisplayItem*, bincode::config::WithOtherEndian<bincode::config::WithOtherLimit<bincode::config::DefaultOptions, bincode::internal::Infinite>, byteorder::LittleEndian,>> 1.716s

bincode::ser::{{impl}}::serialize_field<mut webrender_api::display_list::UnsafeVecWriter*, bincode::config::WithOtherEndian<bincode::config::WithOtherLImit<bincode::config::DefaultOptions, bincode::internal::Infinite>, byteorder::LittleEndian>, webrender_api::display_item::CommonItemProperties> 1.694s

MergeState::ProcessOldNode 1.451s

RetainedDisplayListBuilder::PreProcessDisplayList 1.413s

The time in the serialize functions was always relatively the same to that of ProcessOldNode/PreProcessDisplayList.

After integration with peek_poke I see the following in VTune:

WRRenderBackend#2 (TID: 8464)
ContentProcess (TID: 16040)

RetainedDisplayListBuilder::PreProcessDisplayList 1.982s

MergeState::ProcessOldNode 1.238s

nsIFrame::ClearInvalidationStateBits 1.226s

nsDisplayText::CreateWebRenderCommands 1.076s

nsIFrame::BuildDisplayListForChild 1.048s

MergeState::ResolveNodeIndexesOldToMerged 1.015s

nsDisplayBackgroundColor::RestoreState 0.954s

nsTextFrame::PaintText 0.861s

MergeState::AddNewNode 0.845s

gfxTextRun::Draw 0.730s

floorf 0.708s

mozilla::layers::ClipManager::SwitchItem 0.696s

gfxFont::Draw 0.601s

_security_check_cookie 0.601s

mozilla::layers::WebRenderCommandBuilder::CreateWebRenderCommandsFromDisplayList 0.571s

nsDisplayText::RestoreState 0.559s

webrender_api::display_list::DisplayListBuilder::push_item 0.525s

Where the code to serialize DisplayItem ended up inlined into DisplayListBuilder::push_item. The two top memory intensive functions are combined and fall from #1 to #17 in the stack.

Assignee

Updated

•

5 years ago

Keywords: leave-open

Assignee

Comment 21

•

5 years ago

Attached file Bug 1550640 - P5: Switch to `default` method for peeking enums. (obsolete) — Details

Phabricator Automation

Updated

•

5 years ago

Attachment #9067922 - Attachment is obsolete: true

Assignee

Comment 22

•

5 years ago

Gankro, since using Default to init temporary objects for Peek didn't result in a codegen degradation, combined with peek-poke being for webrender only and not general available on crates.io, I've removed the code that supports using Copy and uninitialized memory.

Phabricator Automation

Updated

•

5 years ago

Attachment #9071781 - Attachment is obsolete: true

Comment 23

•

5 years ago

Pushed by dglastonbury@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/84db3a9838a5 P3: Replace bincode with peek-poke. r=Gankro https://hg.mozilla.org/integration/autoland/rev/2dcbe1372a05 P4: Remove bincode. r=Gankro

Assignee

Updated

•

5 years ago

Keywords: leave-open

Raul Gurzau (:RaulG)

Comment 24

•

5 years ago

Backed out 2 changesets (bug 1550640) for webrenderer bustages on a CLOSED TREE.

Backout link: https://hg.mozilla.org/integration/autoland/rev/d7a0f54d4db28226aa2c457c4867603b74b920e5

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&revision=2dcbe1372a05c3083550924ada21e735081ceca8&selectedJob=256090373

Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=256090373&repo=autoland

Lo 
[task 2019-07-12T01:35:57.551Z] Running the dependency licensing lint...
[task 2019-07-12T01:35:57.551Z]
[taskcluster 2019-07-12 01:35:57.888Z] === Task Finished ===
[taskcluster 2019-07-12 01:35:57.888Z] Unsuccessful task run with exit code: 1 completed in 34.98 seconds

Flags: needinfo?(dglastonbury)

Assignee

Comment 25

•

5 years ago

Fixed lint errors

Flags: needinfo?(dglastonbury)

Bogdan Tara[:bogdan_tara | bogdant]

Comment 26

•

5 years ago

Pushed by dglastonbury@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/46c850f36c6f P3: Replace bincode with peek-poke. r=Gankro https://hg.mozilla.org/integration/autoland/rev/00e5a40ee249 P4: Remove bincode. r=Gankro

Comment 27

•

5 years ago

Backed out 2 changesets (Bug 1550640) for webrender bustages

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=5fd8ae2c6cde7eb09c06baa8c60c0ee2bf05d29d&tochange=594d89724b99bb41a17ddc2511934d0938865811&selectedJob=256103517

Backout link: https://hg.mozilla.org/integration/autoland/rev/594d89724b99bb41a17ddc2511934d0938865811

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=256103517&repo=autoland&lineNumber=430
[task 2019-07-12T03:06:57.236Z] pushd "${GECKO_PATH}/gfx/wr"
[task 2019-07-12T03:06:57.236Z] + pushd /builds/worker/checkouts/gecko/gfx/wr
[task 2019-07-12T03:06:57.236Z] ~/checkouts/gecko/gfx/wr ~
[task 2019-07-12T03:06:57.236Z] CARGOFLAGS="-vv --frozen --target=${TARGET_TRIPLE}"
[task 2019-07-12T03:06:57.236Z] CARGOTESTFLAGS="--no-run"
[task 2019-07-12T03:06:57.236Z] ci-scripts/macos-debug-tests.sh
[task 2019-07-12T03:06:57.236Z] + CARGOFLAGS='-vv --frozen --target=x86_64-apple-darwin'
[task 2019-07-12T03:06:57.236Z] + CARGOTESTFLAGS=--no-run
[task 2019-07-12T03:06:57.236Z] + ci-scripts/macos-debug-tests.sh
[task 2019-07-12T03:06:57.239Z] + CARGOFLAGS='-vv --frozen --target=x86_64-apple-darwin'
[task 2019-07-12T03:06:57.239Z] + CARGOTESTFLAGS=--no-run
[task 2019-07-12T03:06:57.239Z] + pushd webrender_api
[task 2019-07-12T03:06:57.239Z] ~/checkouts/gecko/gfx/wr/webrender_api ~/checkouts/gecko/gfx/wr
[task 2019-07-12T03:06:57.239Z] + cargo check -vv --frozen --target=x86_64-apple-darwin --features ipc
[task 2019-07-12T03:06:57.439Z] error: the lock file /builds/worker/checkouts/gecko/gfx/wr/Cargo.lock needs to be updated but --frozen was passed to prevent this
[taskcluster 2019-07-12 03:07:00.163Z] === Task Finished ===
[taskcluster 2019-07-12 03:07:00.264Z] Artifact "public/build/cargo-test-binaries.tar.bz2" not found at "/builds/worker/artifacts/cargo-test-binaries.tar.bz2"
[taskcluster 2019-07-12 03:07:00.648Z] Unsuccessful task run with exit code: 101 completed in 164.877 seconds

Flags: needinfo?(dglastonbury)

Assignee

Comment 28

•

5 years ago

Found the Wrench failures on try: https://treeherder.mozilla.org/#/jobs?repo=try&collapsedPushes=510609%2C503939%2C494356%2C483091%2C460668&selectedJob=256125213&revision=6d5f565e44e165bce0e4ed3547c49a0fee875ff2

Flags: needinfo?(dglastonbury)