Closed Bug 1363375 Opened 7 years ago Closed 7 years ago

stylo: stop conditionally compiling mServoData

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(2 files, 3 obsolete files)

Part 1 - Add static assert for element sizes on 64-bit. r=smaug
2.42 KB, patch
bholley
: review+
Details | Diff | Splinter Review
Part 2 - Pack nsINode better on 64-bit and stop conditionally compiling mServoData. v3
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.
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+
So does part 1 work currently? Or does it need part 2?
Ah, nm, the second patch shouldn't increase the size
Attachment #8865851 - Flags: review?(bugs) → review+
Attachment #8865850 - Attachment is obsolete: true
(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.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4189e2f808735d6c98b609f72a93d35880ada099

Try push with that approach. I'll flag for review if it's green.
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)
Attachment #8865851 - Attachment is obsolete: true
Attachment #8865851 - Attachment is obsolete: false
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 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)
Attachment #8866289 - Attachment is obsolete: true
Attachment #8866289 - Flags: review?(bugs)
Nice! /me didn't know about target_pointer_width.
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 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.
(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.
Attachment #8866291 - Flags: review?(bugs) → review+
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.
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
https://hg.mozilla.org/mozilla-central/rev/484f671ebb84
https://hg.mozilla.org/mozilla-central/rev/682d5e0d5839
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1366221
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: