The codegen for DisplayItem serialization with bincode is quite horrible. It appears the optimizer is unable to see through the load/store/increment pointer to coalesce load/store of adjacent fields.

mov eax, dword ptr [rsi+0x6c] ;; Load field 1
mov rcx, dword ptr [rsp+0x20] ;; \ Reload UnsafeVecWriter self.0 
mov rdx, qword ptr [rcx]      ;; /
mov dword ptr [rdx], eax      ;; Store field 1
add qword ptr [rcx], 0x4      ;; Increment UnsafeVecWriter self.0
mov eax, dword ptr [rsi+0x70] ;; Load field 2
mov rcx, dword ptr [rsp+0x20] ;; \ Reload UnsafeVecWriter self.0 
mov rdx, qword ptr [rcx]      ;; /
mov dword ptr [rdx], eax      ;; Store field 2
add qword ptr [rcx], 0x4      ;; Increment UnsafeVecWriter self.0
mov eax, dword ptr [rsi+0x74] ;; Load field 3
mov rcx, dword ptr [rsp+0x20] ;; \ Reload UnsafeVecWriter self.0 
mov rdx, qword ptr [rcx]      ;; /
mov dword ptr [rdx], eax      ;; Store field 3
add qword ptr [rcx], 0x4      ;; Increment UnsafeVecWriter self.0

:-(

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.

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.

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)

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.

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?

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.

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

(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.

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.

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.

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

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

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?

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

only two parts have landed

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.

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.

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.

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

Log snippet:
[task 2019-07-12T01:35:56.955Z] ./webrender_api/src/font.rs:358: Line is longer than 120 characters
[task 2019-07-12T01:35:56.986Z]
[task 2019-07-12T01:35:56.988Z] Progress: 97% (940/964)
[task 2019-07-12T01:35:56.988Z] ./webrender_api/src/image.rs:8: use statement contains braces for single import
[task 2019-07-12T01:35:57.024Z]
[task 2019-07-12T01:35:57.028Z] Progress: 97% (941/964)
[task 2019-07-12T01:35:57.028Z] ./webrender_api/src/api.rs:799: Line is longer than 120 characters
[task 2019-07-12T01:35:57.148Z]
[task 2019-07-12T01:35:57.155Z] Progress: 97% (942/964)
[task 2019-07-12T01:35:57.155Z] ./webrender_api/src/display_list.rs:20: encountered whitespace following a use statement
[task 2019-07-12T01:35:57.267Z]
[task 2019-07-12T01:35:57.271Z] Progress: 97% (943/964)
[task 2019-07-12T01:35:57.279Z] Progress: 97% (944/964)
[task 2019-07-12T01:35:57.284Z] Progress: 98% (945/964)
[task 2019-07-12T01:35:57.291Z] Progress: 98% (946/964)
[task 2019-07-12T01:35:57.304Z] Progress: 98% (947/964)
[task 2019-07-12T01:35:57.316Z] Progress: 98% (948/964)
[task 2019-07-12T01:35:57.323Z] Progress: 98% (949/964)
[task 2019-07-12T01:35:57.330Z] Progress: 98% (950/964)
[task 2019-07-12T01:35:57.349Z] Progress: 98% (951/964)
[task 2019-07-12T01:35:57.375Z] Progress: 98% (952/964)
[task 2019-07-12T01:35:57.393Z] Progress: 98% (953/964)
[task 2019-07-12T01:35:57.393Z] Progress: 98% (954/964)
[task 2019-07-12T01:35:57.410Z] Progress: 99% (955/964)
[task 2019-07-12T01:35:57.435Z] Progress: 99% (956/964)
[task 2019-07-12T01:35:57.445Z] Progress: 99% (957/964)
[task 2019-07-12T01:35:57.446Z] Progress: 99% (958/964)
[task 2019-07-12T01:35:57.464Z] Progress: 99% (959/964)
[task 2019-07-12T01:35:57.486Z] Progress: 99% (960/964)
[task 2019-07-12T01:35:57.498Z] Progress: 99% (961/964)
[task 2019-07-12T01:35:57.523Z] Progress: 99% (962/964)
[task 2019-07-12T01:35:57.525Z] Progress: 99% (963/964)
[task 2019-07-12T01:35:57.551Z] Progress: 100% (964/964)
[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

Fixed lint errors

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

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

https://hg.mozilla.org/mozilla-central/rev/df82f4679f43
https://hg.mozilla.org/mozilla-central/rev/0026ebc1bfc1