Closed
Bug 1363375
Opened 8 years ago
Closed 8 years ago
stylo: stop conditionally compiling mServoData
Categories
(Core :: CSS Parsing and Computation, enhancement)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(2 files, 3 obsolete files)
2.42 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
8.74 KB,
patch
|
smaug
:
review+
emilio
:
review+
|
Details | Diff | Splinter Review |
Compiling stylo by default means reserving an extra word on Element to store the Servo data. There was some concern in bug 1362982 that growing Element could negatively impact the l2 cache hit rate, but I can rearrange stuff to make this stuff pack better on 64-bit.
We realistically can't proceed on stylo without mServoData.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8865850 -
Flags: review?(bugs)
Assignee | ||
Comment 2•8 years ago
|
||
Attachment #8865851 -
Flags: review?(bugs)
Comment 3•8 years ago
|
||
Comment on attachment 8865850 [details] [diff] [review]
Part 1 - Add static assert for element size on 64-bit. v1
>+// Verify sizeof(Element) on 64-bit platforms. This should catch most memory
>+// regressions, and is easy to verify locally since most developers are on
>+// 64-bit machines. We use a template rather than a direct static assert so
>+// that the error message actually displays sizeof(Element).
>+template<int a, int b> struct CheckElementSize {
{ should be on its own line.
>+ static_assert(sizeof(void*) != 8 || a == b,
>+ "sizeof(Element) has changed. Be careful about making this bigger!");
>+};
>+CheckElementSize<sizeof(Element), 128> ces;
Technically should be sCes or gCes;
Attachment #8865850 -
Flags: review?(bugs) → review+
Comment 4•8 years ago
|
||
So does part 1 work currently? Or does it need part 2?
Comment 5•8 years ago
|
||
Ah, nm, the second patch shouldn't increase the size
Updated•8 years ago
|
Attachment #8865851 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 6•8 years ago
|
||
Assignee | ||
Comment 7•8 years ago
|
||
Attachment #8865945 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8865850 -
Attachment is obsolete: true
Assignee | ||
Comment 8•8 years ago
|
||
Assignee | ||
Comment 9•8 years ago
|
||
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #2)
> Created attachment 8865851 [details] [diff] [review]
> Part 2 - Pack nsINode better on 64-bit and stop conditionally compiling
> mServoData. v1
Hm, so clang and GCC seem to do the smart thing here, but MSVC doesn't (and bindgen breaks as well). That's probably an indicator that this approach is fragile.
I'll try just conditionally sticking the field directly on nsWrapperCache if we're 64-bit.
Assignee | ||
Comment 10•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4189e2f808735d6c98b609f72a93d35880ada099
Try push with that approach. I'll flag for review if it's green.
Assignee | ||
Comment 11•8 years ago
|
||
Whoops, forgot to commit my local changes.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=26161d78b8464fc75c48bea2ba8b931a2b39f684
Comment 12•8 years ago
|
||
Bindgen bug is https://github.com/servo/rust-bindgen/issues/380, btw.
Assignee | ||
Comment 13•8 years ago
|
||
Assignee | ||
Comment 14•8 years ago
|
||
Green on try. Flagging smaug for the DOM bits, emilio for the servo bits.
Attachment #8866289 -
Flags: review?(emilio+bugs)
Attachment #8866289 -
Flags: review?(bugs)
Assignee | ||
Updated•8 years ago
|
Attachment #8865851 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8865851 -
Attachment is obsolete: false
Assignee | ||
Comment 15•8 years ago
|
||
Comment on attachment 8865851 [details] [diff] [review]
Part 2 - Pack nsINode better on 64-bit and stop conditionally compiling mServoData. v1
># HG changeset patch
># User Bobby Holley <bobbyholley@gmail.com>
>
>Bug 1363375 - Pack nsINode better on 64-bit and stop conditionally compiling mServoData. v1
>
>diff --git a/dom/base/Element.cpp b/dom/base/Element.cpp
>index 8003195266e6..69ca72abf6c2 100644
>--- a/dom/base/Element.cpp
>+++ b/dom/base/Element.cpp
>@@ -1832,19 +1832,17 @@ Element::UnbindFromTree(bool aDeep, bool aNullParent)
>
> ClearInDocument();
>
> // Computed styled data isn't useful for detached nodes, and we'll need to
> // recomputed it anyway if we ever insert the nodes back into a document.
> if (IsStyledByServo()) {
> ClearServoData();
> } else {
>-#ifdef MOZ_STYLO
> MOZ_ASSERT(!HasServoData());
>-#endif
> }
>
> // Editable descendant count only counts descendants that
> // are in the uncomposed document.
> ResetEditableDescendantCount();
>
> if (aNullParent || !mParent->IsInShadowTree()) {
> UnsetFlags(NODE_IS_IN_SHADOW_TREE);
>diff --git a/dom/base/Element.h b/dom/base/Element.h
>index f82ae28e24df..24f995ae5930 100644
>--- a/dom/base/Element.h
>+++ b/dom/base/Element.h
>@@ -166,19 +166,17 @@ public:
> {
> MOZ_ASSERT(mNodeInfo->NodeType() == nsIDOMNode::ELEMENT_NODE,
> "Bad NodeType in aNodeInfo");
> SetIsElement();
> }
>
> ~Element()
> {
>-#ifdef MOZ_STYLO
> NS_ASSERTION(!HasServoData(), "expected ServoData to be cleared earlier");
>-#endif
> }
>
> #endif // MOZILLA_INTERNAL_API
>
> NS_DECLARE_STATIC_IID_ACCESSOR(NS_ELEMENT_IID)
>
> NS_IMETHOD QueryInterface(REFNSIID aIID, void** aInstancePtr) override;
>
>@@ -454,21 +452,17 @@ public:
>
> inline void NoteDirtyDescendantsForServo();
>
> #ifdef DEBUG
> inline bool DirtyDescendantsBitIsPropagatedForServo();
> #endif
>
> bool HasServoData() const {
>-#ifdef MOZ_STYLO
> return !!mServoData.Get();
>-#else
>- MOZ_CRASH("Accessing servo node data in non-stylo build");
>-#endif
> }
>
> void ClearServoData();
>
> /**
> * Gets the custom element data used by web components custom element.
> * Custom element data is created at the first attempt to enqueue a callback.
> *
>@@ -1533,20 +1527,18 @@ private:
> */
> nsRect GetClientAreaRect();
>
> nsIScrollableFrame* GetScrollFrame(nsIFrame **aStyledFrame = nullptr,
> bool aFlushLayout = true);
>
> // Data members
> EventStates mState;
>-#ifdef MOZ_STYLO
> // Per-node data managed by Servo.
> mozilla::ServoCell<ServoNodeData*> mServoData;
>-#endif
> };
>
> class RemoveFromBindingManagerRunnable : public mozilla::Runnable
> {
> public:
> RemoveFromBindingManagerRunnable(nsBindingManager* aManager,
> nsIContent* aContent,
> nsIDocument* aDoc);
>diff --git a/dom/base/nsINode.h b/dom/base/nsINode.h
>index 0d8749a57b4d..df6a687e96b9 100644
>--- a/dom/base/nsINode.h
>+++ b/dom/base/nsINode.h
>@@ -353,19 +353,19 @@ public:
>
> friend class nsNodeUtils;
> friend class nsNodeWeakReference;
> friend class nsNodeSupportsWeakRefTearoff;
> friend class nsAttrAndChildArray;
>
> #ifdef MOZILLA_INTERNAL_API
> explicit nsINode(already_AddRefed<mozilla::dom::NodeInfo>& aNodeInfo)
>- : mNodeInfo(aNodeInfo)
>+ : mBoolFlags(0)
>+ , mNodeInfo(aNodeInfo)
> , mParent(nullptr)
>- , mBoolFlags(0)
> , mNextSibling(nullptr)
> , mPreviousSibling(nullptr)
> , mFirstChild(nullptr)
> , mSubtreeRoot(this)
> , mSlots(nullptr)
> {
> }
> #endif
>@@ -2048,31 +2048,32 @@ public:
> void SetOn##name_(mozilla::dom::EventHandlerNonNull* listener);
> #define TOUCH_EVENT EVENT
> #define DOCUMENT_ONLY_EVENT EVENT
> #include "mozilla/EventNameList.h"
> #undef DOCUMENT_ONLY_EVENT
> #undef TOUCH_EVENT
> #undef EVENT
>
>+private:
>+ // Boolean flags. We put this first so that it can pack with the 32-bit flags
>+ // inherited from nsWrapperCache, which saves a word on 64-bit.
>+ uint32_t mBoolFlags;
>+
> protected:
> static bool Traverse(nsINode *tmp, nsCycleCollectionTraversalCallback &cb);
> static void Unlink(nsINode *tmp);
>
> RefPtr<mozilla::dom::NodeInfo> mNodeInfo;
>
> // mParent is an owning ref most of the time, except for the case of document
> // nodes, so it cannot be represented by nsCOMPtr, so mark is as
> // MOZ_OWNING_REF.
> nsINode* MOZ_OWNING_REF mParent;
>
>-private:
>- // Boolean flags.
>- uint32_t mBoolFlags;
>-
> protected:
> // These references are non-owning and safe, as they are managed by
> // nsAttrAndChildArray.
> nsIContent* MOZ_NON_OWNING_REF mNextSibling;
> nsIContent* MOZ_NON_OWNING_REF mPreviousSibling;
> // This reference is non-owning and safe, since in the case of documents,
> // it is set to null when the document gets destroyed, and in the case of
> // other nodes, the children keep the parents alive.
>
Attachment #8865851 -
Attachment is obsolete: true
Comment 16•8 years ago
|
||
Comment on attachment 8866289 [details] [diff] [review]
Part 2 - Pack nsINode better on 64-bit and stop conditionally compiling mServoData. v2
Review of attachment 8866289 [details] [diff] [review]:
-----------------------------------------------------------------
::: servo/components/style/build_gecko.rs
@@ +281,5 @@
> + // on processor architecture.
> + let bool_flags_location = if ::std::mem::size_of::<usize>() == 8 {
> + "{ n._base._base_1.mBoolFlags }"
> + } else {
> + "{ n.mBoolFlags }"
You don't need to tweak the build script for this. You can add a new file to the `sugar` module, and use #[cfg(target_pointer_width)] to conditionally compile the accessor (or do it directly in the wrapper.rs file).
Attachment #8866289 -
Flags: review?(emilio+bugs)
Assignee | ||
Comment 17•8 years ago
|
||
Attachment #8866291 -
Flags: review?(emilio+bugs)
Attachment #8866291 -
Flags: review?(bugs)
Assignee | ||
Updated•8 years ago
|
Attachment #8866289 -
Attachment is obsolete: true
Attachment #8866289 -
Flags: review?(bugs)
Assignee | ||
Comment 18•8 years ago
|
||
Nice! /me didn't know about target_pointer_width.
Comment 19•8 years ago
|
||
Comment on attachment 8866291 [details] [diff] [review]
Part 2 - Pack nsINode better on 64-bit and stop conditionally compiling mServoData. v3
Review of attachment 8866291 [details] [diff] [review]:
-----------------------------------------------------------------
r=me on the rust bits.
Attachment #8866291 -
Flags: review?(emilio+bugs) → review+
Comment 20•8 years ago
|
||
Comment on attachment 8866291 [details] [diff] [review]
Part 2 - Pack nsINode better on 64-bit and stop conditionally compiling mServoData. v3
Review of attachment 8866291 [details] [diff] [review]:
-----------------------------------------------------------------
But r=me regardless.
::: servo/components/style/gecko/wrapper.rs
@@ +120,5 @@
> + fn bool_flags(&self) -> u32 {
> + (self.0)._base._base_1.mBoolFlags
> + }
> +
> + #[cfg(target_pointer_width = "32")]
Well, presumably you just want #[cfg(not(target_pointer_width = "64"))], just in case.
Assignee | ||
Comment 21•8 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #20)
> Comment on attachment 8866291 [details] [diff] [review]
> Part 2 - Pack nsINode better on 64-bit and stop conditionally compiling
> mServoData. v3
>
> Review of attachment 8866291 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> But r=me regardless.
>
> ::: servo/components/style/gecko/wrapper.rs
> @@ +120,5 @@
> > + fn bool_flags(&self) -> u32 {
> > + (self.0)._base._base_1.mBoolFlags
> > + }
> > +
> > + #[cfg(target_pointer_width = "32")]
>
> Well, presumably you just want #[cfg(not(target_pointer_width = "64"))],
> just in case.
The gecko code statically asserts that we're 32-bit if not 64-bit, so I thought I'd mirror it here.
Updated•8 years ago
|
Attachment #8866291 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 22•8 years ago
|
||
Thanks for the reviews! Turns out the VCS sync service is down, so I'll have to wait for that to be fixed before landing.
Comment 23•8 years ago
|
||
Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/484f671ebb84
Add static assert for element sizes on 64-bit. r=smaug
https://hg.mozilla.org/integration/autoland/rev/682d5e0d5839
Pack nsINode better on 64-bit and stop conditionally compiling mServoData. r=smaug
Comment 24•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/484f671ebb84
https://hg.mozilla.org/mozilla-central/rev/682d5e0d5839
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•