Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upEnable refcounting arbitrary DOM types #4057
Conversation
hoppipolla-critic-bot
commented
Nov 21, 2014
|
Critic review: https://critic.hoppipolla.co.uk/r/3237 This is an external review system which you may optionally use for the code review of your pull request. In order to help critic track your changes, please do not make in-place history rewrites (e.g. via |
|
Note: #4173 kind of obsoletes the lint. It currently doesn't check that the first field is a reflector (just that the first field is another DOM struct OR it just contains a reflector), but it's a short fix. Should I incorporate the improvement to reflector checking in that PR? |
This comment has been minimized.
This comment has been minimized.
|
r=Ms2ger,Manishearth |
This comment has been minimized.
This comment has been minimized.
|
saw approval from Ms2ger |
This comment has been minimized.
This comment has been minimized.
|
merging jdm/servo/refcountdom = 9a7cd31 into auto |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
all tests pass: |
This comment has been minimized.
This comment has been minimized.
|
fast-forwarding master to auto = 2c259f4 |
This replaces the specialized TrustedXHRAddress and TrustedWorkerAddress code that was used for the same purpose. A non-zero refcount pins the given DOM object's reflector and prevents it from being GCed even when there are no other outstanding references visible to SpiderMonkey. This will enable us to implement asynchronous operations that refer to particular DOM objects (such as "queue a task to fire a simple event named load at the iframe element" from the spec) safely and conveniently, and paves the way for things like asynchronous network responses. Some concerns about the resulting size of XHR progress messages have been expressed, but I believe optimizations to reduce that can be implemented in subsequent PRs. r? @Ms2ger - note in particular the changes to the worker lifetime code. I couldn't figure out how to achieve an identical lifetime to the previous addref/release pairing, and I also was having trouble figuring out why the existing setup was safe. The new implementation now holds the main script task Worker object alive via the TrustedWorkerAddress field in the dedicated worker global scope, which is a significant difference.
bors-servo
merged commit 9a7cd31
into
servo:master
As noted in servo#4057, this commit optimizes the size of Trusted<T> so that XHR progress messages do not have to be too heavy, considering that a large number of progress messages will be generated. This commit also replaces the mutex based reference counting with one based on atomic integer operations.
As noted in servo#4057, this commit optimizes the size of Trusted<T> so that XHR progress messages do not have to be too heavy, considering that a large number of progress messages will be generated. This commit also replaces the mutex based reference counting with one based on atomic integer operations.
jdm commentedNov 21, 2014
This replaces the specialized TrustedXHRAddress and TrustedWorkerAddress code that was used for the same purpose. A non-zero refcount pins the given DOM object's reflector and prevents it from being GCed even when there are no other outstanding references visible to SpiderMonkey. This will enable us to implement asynchronous operations that refer to particular DOM objects (such as "queue a task to fire a simple event named load at the iframe element" from the spec) safely and conveniently, and paves the way for things like asynchronous network responses.
Some concerns about the resulting size of XHR progress messages have been expressed, but I believe optimizations to reduce that can be implemented in subsequent PRs.
r? @Ms2ger - note in particular the changes to the worker lifetime code. I couldn't figure out how to achieve an identical lifetime to the previous addref/release pairing, and I also was having trouble figuring out why the existing setup was safe. The new implementation now holds the main script task Worker object alive via the TrustedWorkerAddress field in the dedicated worker global scope, which is a significant difference.