Closed
Bug 1363572
Opened 7 years ago
Closed 7 years ago
stylo: ServoStyleSet's mUniqueCountId needs to be 64-bit
Categories
(Core :: CSS Parsing and Computation, enhancement)
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: bradwerth)
References
Details
Attachments
(2 files, 1 obsolete file)
Overflowing a 32-bit counter really doesn't take very long on modern processors, unfortunately, and this counter is completely under page control as far as I can tell.
Comment 1•7 years ago
|
||
We could also switch the ID to be the object identity of the Gecko-side object I think. Would that work?
Comment 2•7 years ago
|
||
Either of those options sound OK.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8866462 -
Flags: review?(cam)
Attachment #8866463 -
Flags: review?(cam)
Attachment #8866464 -
Flags: review?(cam)
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8866462 [details] Bug 1363572 Part 1: Servo-side change stylesheet_set entry unique_id to u64. https://reviewboard.mozilla.org/r/138094/#review141410
Attachment #8866462 -
Flags: review?(cam) → review+
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8866463 [details] Bug 1363572 Part 2: Change ServoStyleSet to use uint64_t for entry unique IDs. https://reviewboard.mozilla.org/r/138096/#review141412
Attachment #8866463 -
Flags: review?(cam) → review+
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8866464 [details] Bug 1363572 Part 3: Change ServoStyleSet to use sheet addresses as unique IDs. https://reviewboard.mozilla.org/r/138098/#review141418 Would like to see another version with these things addressed. It might make sense to make the storage of the unique ID on the Servo side be usize, so that it's pointer sized. But I think there are problems with the FFI generation for C++ functions using size_t -- IIRC because it's a typedef to an appropriately sized integer, rust-bindgen just uses the sized C type for the platform it was run on in the generated rust headers, which isn't really compatible with having bindings checked in to the tree. So it's not a big deal to leave StylesheetSetEntry::unique_id and the FFI interface as using u64. ::: layout/style/ServoStyleSet.h:405 (Diff revision 1) > uint64_t FindSheetOfType(SheetType aType, > ServoStyleSheet* aSheet); Do we need this function any more? Seems like it will just return aSheet's pointer value now. ::: layout/style/ServoStyleSet.h:408 (Diff revision 1) > uint64_t PrependSheetOfType(SheetType aType, > - ServoStyleSheet* aSheet, > + ServoStyleSheet* aSheet); > - uint64_t aReuseUniqueID = 0); > > uint64_t AppendSheetOfType(SheetType aType, > - ServoStyleSheet* aSheet, > + ServoStyleSheet* aSheet); > - uint64_t aReuseUniqueID = 0); > > uint64_t InsertSheetOfType(SheetType aType, > ServoStyleSheet* aSheet, > - uint64_t aBeforeUniqueID, > + uint64_t aBeforeUniqueID); I think you may as well remove the uin64_t return values from these methods, since at all the call sites, we can just directly use the ServoStyleSheet pointer value as the unique ID. ::: layout/style/ServoStyleSet.h:422 (Diff revision 1) > > uint64_t RemoveSheetOfType(SheetType aType, > ServoStyleSheet* aSheet); > > struct Entry { > uint64_t uniqueID; If we're using the address of the ServoStyleSheet object as the unique ID, we no longer need to store a separate uniqueID field, and we can go back to mEntries being arrays of RefPtr<ServoStyleSheet>. ::: layout/style/ServoStyleSet.cpp:1054 (Diff revision 1) > ServoStyleSet::PrependSheetOfType(SheetType aType, > - ServoStyleSheet* aSheet, > + ServoStyleSheet* aSheet) > - uint64_t aReuseUniqueID) > { > Entry* entry = mEntries[aType].InsertElementAt(0); > - entry->uniqueID = aReuseUniqueID ? aReuseUniqueID : ++mUniqueIDCounter; > + entry->uniqueID = (uint64_t)(void *)(aSheet); Slight preference to using C++ casts here: entry->uniqueID = reinterpret_cast<uintptr_t>(aSheet); but as mentioned I don't think you need to store uniqueID explicitly any more.
Attachment #8866464 -
Flags: review?(cam) → review-
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8866464 [details] Bug 1363572 Part 3: Change ServoStyleSet to use sheet addresses as unique IDs. https://reviewboard.mozilla.org/r/138098/#review141418 I couldn't find a better solution through the various issues raised here. I went with the recommendation in the final sentence: "So it's not a big deal to leave StylesheetSetEntry::unique_id and the FFI interface as using u64." > Slight preference to using C++ casts here: > > entry->uniqueID = reinterpret_cast<uintptr_t>(aSheet); > > but as mentioned I don't think you need to store uniqueID explicitly any more. Encapsulated this conversion into a helper function defined near the start of the file.
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8866464 [details] Bug 1363572 Part 3: Change ServoStyleSet to use sheet addresses as unique IDs. https://reviewboard.mozilla.org/r/138098/#review141876 This is looking good, thanks! r=me with comments below addressed. ::: layout/style/ServoStyleSet.cpp:32 (Diff revision 2) > #include "nsStyleSet.h" > > using namespace mozilla; > using namespace mozilla::dom; > > +uint64_t UniqueIDForSheet(ServoStyleSheet* aSheet) Make this "static inline". And can you add a short comment in here describing how we use the pointer value (as a uint64_t) as a sheet's unique ID for tracking on the Servo side, rather than bothering to generate unique ID numbers. ::: layout/style/ServoStyleSet.cpp:576 (Diff revision 2) > - // If we were already tracking aSheet, the newUniqueID will be the same > - // as the oldUniqueID. In that case, Servo will remove aSheet from its > + // If we were already tracking aSheet, uniqueID will be the same > + // as the old uniqueID. In that case, Servo will remove aSheet from its > // original position as part of the call to Servo_StyleSet_AppendStyleSheet. Maybe we should reword this comment now that we don't have an Entry struct with a "uniqueID" member. I guess this important thing to mention is that the Servo_StyleSet_AppendStyleSheet call will handle first removing the sheet, if it's already present. ::: layout/style/ServoStyleSet.cpp:602 (Diff revision 2) > - // If we were already tracking aSheet, the newUniqueID will be the same > - // as the oldUniqueID. In that case, Servo will remove aSheet from its > + // If we were already tracking aSheet, uniqueID will be the same > + // as the old uniqueID. In that case, Servo will remove aSheet from its > // original position as part of the call to Servo_StyleSet_PrependStyleSheet. Similarly here. ::: layout/style/ServoStyleSet.cpp:1079 (Diff revision 2) > -uint64_t > +void > ServoStyleSet::InsertSheetOfType(SheetType aType, > ServoStyleSheet* aSheet, > - uint64_t aBeforeUniqueID, > + uint64_t aBeforeUniqueID) > - uint64_t aReuseUniqueID) > { > - for (uint32_t i = 0; i < mEntries[aType].Length(); ++i) { > - if (mEntries[aType][i].uniqueID == aBeforeUniqueID) { > - Entry* entry = mEntries[aType].InsertElementAt(i); > - entry->uniqueID = aReuseUniqueID ? aReuseUniqueID : ++mUniqueIDCounter; > + for (uint32_t i = 0; i < mSheets[aType].Length(); ++i) { > + if (UniqueIDForSheet(mSheets[aType][i]) == aBeforeUniqueID) { > + mSheets[aType].InsertElementAt(i, aSheet); > + return; > - entry->sheet = aSheet; > - return entry->uniqueID; > } > } > - return 0; > } I feel like it's a bit odd that on the C++ side we now use the unique ID for more than just passing it off to the Servo side. So I think this function here would make just as much sense with the last argument being |ServoStyleSheet* aBeforeSheet|.
Attachment #8866464 -
Flags: review?(cam) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8866462 -
Attachment is obsolete: true
Comment 16•7 years ago
|
||
Pushed by bwerth@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/81b863299c2a Part 2: Change ServoStyleSet to use uint64_t for entry unique IDs. r=heycam https://hg.mozilla.org/integration/autoland/rev/2b0fa5a0c30c Part 3: Change ServoStyleSet to use sheet addresses as unique IDs. r=heycam
Comment 17•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/81b863299c2a https://hg.mozilla.org/mozilla-central/rev/2b0fa5a0c30c
Status: NEW → RESOLVED
Closed: 7 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
•