Removed mutex from Trusted, used weak references #12899

asajeffrey · 2016-08-17T00:39:31Z

This PR aims to improve performance by:

Removing a mutex from each Trusted DOM object.
Replacing GC messaging by a use of Weak refcounts.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes do not require tests because they are addressing perf, not correctness.

This change is


        Removed mutex from Trusted, used weak references rather than messages…

… to gc dead references.

highfive · 2016-08-17T00:39:34Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/script_thread.rs, components/script/dom/bindings/refcounted.rs, components/script/script_runtime.rs, components/script/dom/serviceworkerglobalscope.rs, components/script/dom/dedicatedworkerglobalscope.rs

highfive · 2016-08-17T00:39:35Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

asajeffrey · 2016-08-17T00:40:48Z

@jdm is on PTO I believe. r? @Manishearth

Manishearth · 2016-08-17T03:33:15Z

components/script/dom/bindings/refcounted.rs

+fn remove_nulls<K: Eq + Hash + Clone, V> (table: &mut HashMap<K, Weak<V>>) {
+    let to_remove: Vec<K> =
+        table.iter()
+        .filter(|&(_, value)| Weak::upgrade(value).is_none())


Ick. It would be nice if there was an API for this, as it stands we needlessly bump and unbump the refcount.

I can't see how to do it without racing, though maybe that's fine?

Yep, that's why that API doesn't exist, the synchronization semantics need to be decided by the caller.

As far as racing here goes, that's okay -- a few extra references hanging around as live when they aren't is nbd. It will get caught in the next pass.

mumbles something about ephemeron tables

I don't understand why we can't use the entry API here, instead of collecting in an intermediate Vec.

Yes, it would be nice if there was an if_upgradeable that didn't actually do the upgrade. It's racy, but in this case the race is benign, as it just results in gc being delayed.

@nox: the intermediate result is needed because we can't do the remove in the middle of iterating over the hash map.

Manishearth · 2016-08-17T03:37:10Z

lgtm, much nicer code now

@Ms2ger sanity-check?

emilio · 2016-08-17T03:48:39Z

components/script/dom/bindings/refcounted.rs

-                refcount.clone()
-            }
+            Occupied(mut entry) => match entry.get().upgrade() {
+                Some(refcount) => refcount.clone(),


You don't need this clone, right?

Yeah, that's not necessary

Ms2ger · 2016-08-17T07:46:26Z

Have you measured the perf impact?


        Removed unnecessary clone.

asajeffrey · 2016-08-17T15:18:31Z

@Ms2ger So far I've not seen observable differences on just timing the executable. I'll run gperf and see if it reports anything.

asajeffrey · 2016-08-17T16:50:17Z

Rather annoyingly, running with the google perf tools causes panics, probably due to timing changes:

LD_PRELOAD=/usr/lib/libprofiler.so.0 CPUPROFILE=servo-cpu.log ./target/release/servo --exit http://twitter.com/
JQMIGRATE: Migrate is installed with logging active, version 1.4.1
called `Result::unwrap()` on an `Err` value: RecvError (thread LayoutThread PipelineId { namespace_id: PipelineNamespaceId(1), index: PipelineIndex(5) }, at ../src/libcore/result.rs:788)
called `Result::unwrap()` on an `Err` value: RecvError (thread LayoutThread PipelineId { namespace_id: PipelineNamespaceId(1), index: PipelineIndex(1) }, at ../src/libcore/result.rs:788)
ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
called `Result::unwrap()` on an `Err` value: RecvError (thread LayoutThread PipelineId { namespace_id: PipelineNamespaceId(1), index: PipelineIndex(2) }, at ../src/libcore/result.rs:788)
ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvErrorcalled `Result::unwrap()` on an `Err` value: RecvError (thread ScriptThread PipelineId { namespace_id: PipelineNamespaceId(1), index: PipelineIndex(4) }, at ../src/libcore/result.rs:788)

called `Result::unwrap()` on an `Err` value: RecvError (thread LayoutThread PipelineId { namespace_id: PipelineNamespaceId(1), index: PipelineIndex(0) }, at ../src/libcore/result.rs:788)
ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
called `Result::unwrap()` on an `Err` value: RecvError (thread ScriptThread PipelineId { namespace_id: PipelineNamespaceId(1), index: PipelineIndex(1) }, at ../src/libcore/result.rs:788)
ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvErrorcalled `Result::unwrap()` on an `Err` value: RecvError (thread LayoutThread PipelineId { namespace_id: PipelineNamespaceId(1), index: PipelineIndex(3) }, at ../src/libcore/result.rs:788)

called `Result::unwrap()` on an `Err` value: RecvError (thread LayoutThread PipelineId { namespace_id: PipelineNamespaceId(1), index: PipelineIndex(4) }, at ../src/libcore/result.rs:788)
ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
called `Result::unwrap()` on an `Err` value: RecvError (thread ScriptThread PipelineId { namespace_id: PipelineNamespaceId(1), index: PipelineIndex(0) }, at ../src/libcore/result.rs:788)
ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
called `Result::unwrap()` on an `Err` value: RecvError (thread ScriptThread PipelineId { namespace_id: PipelineNamespaceId(1), index: PipelineIndex(5) }, at ../src/libcore/result.rs:788)
ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvErrorcalled `Result::unwrap()` on an `Err` value: RecvError (thread ScriptThread PipelineId { namespace_id: PipelineNamespaceId(1), index: PipelineIndex(2) }, at ../src/libcore/result.rs:788)

ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvErrorcalled `Result::unwrap()` on an `Err` value: RecvError (thread LayoutThread PipelineId { namespace_id: PipelineNamespaceId(0), index: PipelineIndex(0) }, at ../src/libcore/result.rs:788)

ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvErrorcalled `Result::unwrap()` on an `Err` value: RecvError (thread ScriptThread PipelineId { namespace_id: PipelineNamespaceId(1), index: PipelineIndex(3) }, at ../src/libcore/result.rs:788)
called `Result::unwrap()` on an `Err` value: RecvError (thread ImageCacheThread, at ../src/libcore/result.rs:788)

ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
ERROR:servo: called `Result::unwrap()` on an `Err` value: RecvError
called `Result::unwrap()` on an `Err` value: Error { repr: Os { code: 32, message: "Broken pipe" } } (thread ScriptThread PipelineId { namespace_id: PipelineNamespaceId(0), index: PipelineIndex(0) }, at ../src/libcore/result.rs:788)
ERROR:servo: called `Result::unwrap()` on an `Err` value: Error { repr: Os { code: 32, message: "Broken pipe" } }

Valgrind produces an error:

valgrind --tool=callgrind ./target/release/servo-master 
==11528== Callgrind, a call-graph generating cache profiler
==11528== Copyright (C) 2002-2015, and GNU GPL'd, by Josef Weidendorfer et al.
==11528== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==11528== Command: ./target/release/servo-master
==11528== 
==11528== For interactive control, run 'callgrind_control -h'.
==11528== Warning: noted but unhandled ioctl 0x6458 with no size/direction hints.
==11528==    This could cause spurious value errors to appear.
==11528==    See README_MISSING_SYSCALL_OR_IOCTL for guidance on writing a proper wrapper.

vex: the `impossible' happened:
   isZeroU
vex storage: T total 2388863016 bytes allocated
vex storage: P total 640 bytes allocated

valgrind: the 'impossible' happened:
   LibVEX called failure_exit().

This is making profiling difficult.

asajeffrey · 2016-08-17T22:29:42Z

OK, I learned how to use perf diff:

# Event 'cycles'
#
# Baseline    Delta  Shared Object             Symbol                                                                                         
# ........  .......  ........................  .............................................................................................................................................................................................................................
#
    13.39%   +0.22%  [unknown]                 [k] 0xffffffff810034a8                                                                                                                                                                                                       
     2.85%           servo-master              [.] _ZN53_$LT$$u5b$T$u5d$$u20$as$u20$core..slice..SliceExt$GT$3len17hf0fcd69cba1a6f63E                                                                                                                                       
     1.72%           servo-master              [.] _ZN39_$LT$collections..vec..Vec$LT$T$GT$$GT$17extend_from_slice17hd5647c7a4e71bce8E                                                                                                                                      
     1.71%           servo-master              [.] _ZN4core3ptr33_$LT$impl$u20$$BP$const$u20$T$GT$6offset17hcd0266fddffecdf0E                                                                                                                                               
     1.51%           servo-master              [.] _ZN4core5slice95_$LT$impl$u20$core..ops..Index$LT$core..ops..Range$LT$usize$GT$$GT$$u20$for$u20$$u5b$T$u5d$$GT$5index17h17a0529ad270abc8E                                                                                
     1.45%           servo-master              [.] _ZN63_$LT$core..ptr..Unique$LT$T$GT$$u20$as$u20$core..ops..Deref$GT$5deref17h8838acf83c79c609E                                                                                                                           
     1.36%   -0.05%  libxcb.so.1.1.0           [.] 0x0000000000009b40                                                                                                                                                                                                       
     1.35%           servo-master              [.] _ZN47_$LT$rand..XorShiftRng$u20$as$u20$rand..Rng$GT$8next_u3217h148133191adba9cdE                                                                                                                                        
     1.27%           servo-master              [.] _ZN4core3ptr31_$LT$impl$u20$$BP$mut$u20$T$GT$6offset17hda601c582351267eE                                                                                                                                                 
     1.23%   +0.02%  libc-2.21.so              [.] __memcpy_avx_unaligned                                                                                                                                                                                                   
     1.18%   -0.08%  libpthread-2.21.so        [.] pthread_mutex_lock                                                                                                                                                                                                       
     1.09%           servo-master              [.] _ZN68_$LT$core..nonzero..NonZero$LT$T$GT$$u20$as$u20$core..ops..Deref$GT$5deref17h3da8882f7b2a4ce7E                                                                                                                      
     1.05%   -0.03%  libpthread-2.21.so        [.] pthread_mutex_unlock                                                                                                                                                                                                     
     1.04%           servo-master              [.] _ZN4core5slice14from_raw_parts17h289116082bf3e774E                                                                                                                                                                       
     1.03%   -0.10%  libX11.so.6.3.0           [.] 0x000000000002b350                                                                                                                                                                                                       
     1.01%           servo-master              [.] _ZN4core3ptr31_$LT$impl$u20$$BP$mut$u20$T$GT$7is_null17hee09f69cb916a056E                                                                                                                                                
     0.99%   -0.02%  libpthread-2.21.so        [.] 0x0000000000010229

We're seeing mutex locking going down from 1.18% of cpu to 1.10%. Not exactly a huge drop, but a drop.

This is from `perf record ./target/debug/servo -x http://google.com/'. It is doing a bit more memcpy, probably due to trusted objects being GCd later, so the hash table ends up being a bit larger.

Ms2ger · 2016-08-18T12:23:55Z

Please do rewrite the big documentation comment at the top of the file; I have no clue what's going on here.

asajeffrey · 2016-08-18T14:58:42Z

Will do.


        Updated refcounted rustdoc comments.

asajeffrey · 2016-08-18T15:41:55Z

Documentation rewritten.

KiChjang · 2016-08-18T17:32:48Z

components/script/dom/bindings/refcounted.rs

    LIVE_REFERENCES.with(|ref r| {
        let r = r.borrow();
        let live_references = r.as_ref().unwrap();
-        let table = live_references.table.borrow();
+        let mut table = live_references.table.borrow_mut();
+        remove_nulls(&mut table);


If I'm reading this correctly, does it mean that the JS GC will call into this code and clean up the RCs on the LiveDOMReferences hash table? And this new process would make the dance between dropping -> sending cleanup message to the script thread -> doing the actual cleanup unnecessary?

That's the idea, it's replacing the GC message to the owner thread by a use of weak references. There is a slight extra cost, since we only reclaim space in the hash table when GC happens, rather than when the pointer is dropped. (This is a case where something like Java's WeakHashMap would be useful.) But that cost is made up for by saving on messaging.

There is already precedent for such an idiom in Servo itself with the JSTraceable impl of WeakRangeVec in range.rs.

Hmm, goes and looks... there's a WeakRef implementation in weakref.rs, including an implementation of retain_alive. It's slightly different I think, because Trusted can be sent to other threads, so doesn't have access to the reflector. Also, I can't see an impl of JSTraceable for WeakRefVec, is the expectation that any use of WeakRefVec will impl JSTraceable by hand? WeakRangeVec does.

I didn't say we could use it for Trusted, I was just saying we have a precedent for cleaning stuff up on GC trace.

As for your question, WeakRefVec doesn't implement JSTraceable itself because cleaning up needs mutable access to it.

Oh that's interesting, JSTraceable only provides immutable access to the object. Does mean you're limited in terms of which tracing algorithms you use, e.g. no pointer reversal.

nox · 2016-08-29T10:00:44Z

Superseded by #13106.

Removed mutex from Trusted, used weak references rather than messages…

Loading status checks…

b341885

… to gc dead references.

highfive assigned jdm Aug 17, 2016

highfive added the S-awaiting-review label Aug 17, 2016

asajeffrey added I-perf-bloat A-content/script S-awaiting-review and removed S-awaiting-review labels Aug 17, 2016

highfive assigned Manishearth and unassigned jdm Aug 17, 2016

Manishearth reviewed Aug 17, 2016
View changes

emilio reviewed Aug 17, 2016
View changes

Removed unnecessary clone.

Loading status checks…

e7454f9

asajeffrey mentioned this pull request Aug 17, 2016

Running wtih Google perf tools causes panic #12908

Open

Updated refcounted rustdoc comments.

Loading status checks…

3c468b5

KiChjang reviewed Aug 18, 2016
View changes

nox added the S-needs-squash label Aug 26, 2016

nox closed this Aug 29, 2016

servo / servo

Removed mutex from Trusted, used weak references #12899

Removed mutex from Trusted, used weak references #12899

asajeffrey commented Aug 17, 2016 •

edited by larsbergstrom

highfive commented Aug 17, 2016

highfive commented Aug 17, 2016

asajeffrey commented Aug 17, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth commented Aug 17, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Ms2ger commented Aug 17, 2016

asajeffrey commented Aug 17, 2016

asajeffrey commented Aug 17, 2016

asajeffrey commented Aug 17, 2016

Ms2ger commented Aug 18, 2016

asajeffrey commented Aug 18, 2016

asajeffrey commented Aug 18, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nox commented Aug 29, 2016

servo / servo

Join GitHub today

Removed mutex from Trusted, used weak references #12899

Removed mutex from Trusted, used weak references #12899

Conversation

asajeffrey commented Aug 17, 2016 • edited by larsbergstrom

highfive commented Aug 17, 2016

highfive commented Aug 17, 2016

asajeffrey commented Aug 17, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth commented Aug 17, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Ms2ger commented Aug 17, 2016

asajeffrey commented Aug 17, 2016

asajeffrey commented Aug 17, 2016

asajeffrey commented Aug 17, 2016

Ms2ger commented Aug 18, 2016

asajeffrey commented Aug 18, 2016

asajeffrey commented Aug 18, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nox commented Aug 29, 2016

asajeffrey commented Aug 17, 2016 •

edited by larsbergstrom