Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2814603002:
Move LayoutObject to satellite NodeLayoutData that hangs from a Node. (Closed)
DescriptionMove LayoutObject to satellite NodeLayoutData that hangs from a Node.
This patch moves the LayoutObject to be stored on LayoutData which hangs
off a Node instead of being stored on the DataUnion (either directly or
on a NodeRareData.)
This is the first step in adding more Rendering information on a satellite
object thereby not increasing the size of a Node but allowing us to store
more information nonetheless.
BUG=595137
Review-Url: https://codereview.chromium.org/2814603002
Cr-Commit-Position: refs/heads/master@{#464622}
Committed: https://chromium.googlesource.com/chromium/src/+/2f631c11f8e8f80c2373a6da4e4c24f7337ea55e
Patch Set 1 #
Total comments: 9
Patch Set 2 : Question #
Total comments: 2
Patch Set 3 : Incorporate esprehn@ feedback #
Total comments: 15
Patch Set 4 : Review's comments #
Total comments: 8
Patch Set 5 : Post lgtm review #
Total comments: 8
Patch Set 6 : esprehn comments #
Messages
Total messages: 37 (23 generated)
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
nainar@chromium.org changed reviewers: + esprehn@chromium.org, szager@chromium.org
Hi both, Implemented the patch we were talking about. On second thought there is some inconsistency in the way I access the LayoutData (directly or through a getter.) Cleaning that up. Let me know what you think.
https://codereview.chromium.org/2814603002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/2814603002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Node.cpp:315: ElementRareData::Create(data_.layout_data_->GetLayoutObject()); You need to store the layout_data_ inside the ElementRareData, not the LayoutObject, otherwise the union doesn't work. Also this would crash when layout_data_ is null. This should be ElementRareData::Create(data_.layout_data_) and you need some kind of EnsureLayoutData(), I also think we want to avoid any new branches, which means we need to use some kind of shared empty LayoutData instance to Node::layoutObject() doesn't need another branch, and setLayoutObject() (or any setter) checks if Node::layout_data_ == NodeLayoutData::sharedEmptyData() and allocates a non shared one if it is. https://codereview.chromium.org/2814603002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Node.h (right): https://codereview.chromium.org/2814603002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Node.h:100: class LayoutData { NodeLayoutData maybe? LayoutData is pretty generic, it's also a bit unfortunate that this thing is both style and layout, I miss the days when we just called this stuff Rendering :P But the bike shed is not super important. https://codereview.chromium.org/2814603002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Node.h:102: LayoutData(LayoutObject* layout_object) : layout_object_(layout_object) {} explicit on all single arg constructors unless you want to allow implicit construction, you probably don't want that here. https://codereview.chromium.org/2814603002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/NodeRareData.cpp (right): https://codereview.chromium.org/2814603002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/NodeRareData.cpp:80: CHECK(!GetLayoutData()->GetLayoutObject()); leave this assert alone, GetLayoutObject() needs to look through the data
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Just adding a note for IRL conversation https://codereview.chromium.org/2814603002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/2814603002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Node.cpp:315: ElementRareData::Create(data_.layout_data_->GetLayoutObject()); On 2017/04/10 at 22:20:12, esprehn wrote: > You need to store the layout_data_ inside the ElementRareData, not the LayoutObject, otherwise the union doesn't work. Also this would crash when layout_data_ is null. > > This should be ElementRareData::Create(data_.layout_data_) > > and you need some kind of EnsureLayoutData(), I also think we want to avoid any new branches, which means we need to use some kind of shared empty LayoutData instance to Node::layoutObject() doesn't need another branch, and setLayoutObject() (or any setter) checks if Node::layout_data_ == NodeLayoutData::sharedEmptyData() and allocates a non shared one if it is. Referring back to IRL conversation. We shouldn't store the LayoutObject on RareData anymore and just on NodeLayoutData.
Posting code here cause I think I misunderstood you. We can't not store the LayoutObject information on NodeRareDataBase. What if the DataUnion is a NodeRareDataBase where do we store the LayoutObject in that case? https://codereview.chromium.org/2814603002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Node.h (right): https://codereview.chromium.org/2814603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.h:925: NodeLayoutData* node_layout_data_; If the DataUnion is a NodeLayoutData we then have a LayoutObject stored on the Node through this NodeLayoutData https://codereview.chromium.org/2814603002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.h:926: NodeRareDataBase* rare_data_; However, if we have a NodeRareDataBase we don't have a LayoutObject? This seems wrong. I think we will have to retain storing the LayoutObject on NodeRareDataBase. Either directly or via an instance of NodeLayoutData.
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Elliott, PTAL? Thanks! https://codereview.chromium.org/2814603002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/2814603002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Node.cpp:315: ElementRareData::Create(data_.layout_data_->GetLayoutObject()); NodeRareDataBase and its descendants now store NodeLayoutData. Added the static empty_shared_data instance of NodeLayoutData https://codereview.chromium.org/2814603002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Node.h (right): https://codereview.chromium.org/2814603002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Node.h:100: class LayoutData { On 2017/04/10 at 22:20:12, esprehn wrote: > NodeLayoutData maybe? LayoutData is pretty generic, it's also a bit unfortunate that this thing is both style and layout, I miss the days when we just called this stuff Rendering :P But the bike shed is not super important. Done. https://codereview.chromium.org/2814603002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Node.h:102: LayoutData(LayoutObject* layout_object) : layout_object_(layout_object) {} On 2017/04/10 at 22:20:12, esprehn wrote: > explicit on all single arg constructors unless you want to allow implicit construction, you probably don't want that here. Done. https://codereview.chromium.org/2814603002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/NodeRareData.cpp (right): https://codereview.chromium.org/2814603002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/NodeRareData.cpp:80: CHECK(!GetLayoutData()->GetLayoutObject()); On 2017/04/10 at 22:20:12, esprehn wrote: > leave this assert alone, GetLayoutObject() needs to look through the data NodeRareDataBase stores a pointer to NodeLayoutData which has a LayoutObject. This needs to be there. https://codereview.chromium.org/2814603002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Node.h (right): https://codereview.chromium.org/2814603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.h:595: if (HasRareData()) { Can make this less complicated by adding a EnsureLayoutObject method on NodeLayoutData, NodeLayoutData* NodeLayoutData::EnsureLayoutObject(layout_object) { if this == &NodeLayoutData::SharedEmptyData() return new NodeLayoutData(layout_object) else SetLayoutObject(layout_object); }
https://codereview.chromium.org/2814603002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Node.h (right): https://codereview.chromium.org/2814603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.h:108: layout_object_ = layout_object; DCHECK_NE(&SharedEmptyData(), this); https://codereview.chromium.org/2814603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.h:122: return node_layout_data_ ? node_layout_data_ The point of the SharedEmptyData() is that we don't need this null check, we should always either have node_layout_data_ be SharedEmptyData() or a unique one, it's not nullable. https://codereview.chromium.org/2814603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.h:125: void SetNodeLayoutData(NodeLayoutData* node_layout_data) { DCHECK(node_layout_data) https://codereview.chromium.org/2814603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.h:134: NodeLayoutData* node_layout_data_; you need a destructor for NodeRareDataBase and inside ~Node, that does: if (node_layout_data_ && node_layout_data_ != SharedEmptyData()) delete node_layout_data_; or you need to make NodeLayoutData GarbageCollected and then this should all work automatically since the static Persistent in SharedEmptyData would keep it alive. Making it garbage collected is less code which is nice, you'll need to modify the tracing function here: https://cs.chromium.org/webrtc/src/third_party/WebKit/Source/core/dom/Node.cp... Right now you're leaking this object though. https://codereview.chromium.org/2814603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.h:594: void SetLayoutObject(LayoutObject* layout_object) { This function has become so big, I'd suggest moving out of line into the .cpp file. The one function call is unlikely to be a perf issue since we're already doing something so expensive when we call setLayoutObject. https://codereview.chromium.org/2814603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.h:596: data_.rare_data_->GetNodeLayoutData() == we should put this in a method on NodeLayoutData like bool IsShared() https://codereview.chromium.org/2814603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.h:603: if (data_.node_layout_data_ == &NodeLayoutData::SharedEmptyData()) if (data_.node_layout_data_->IsShared())
Description was changed from ========== Move LayoutObject to satellite LayoutData that hangs from a Node. This patch moves the LayoutObject to be stored on LayoutData which hangs off a Node instead of being stored on the DataUnion (either directly or on a NodeRareData.) This is the first step in adding more Rendering information on a satellite object thereby not increasing the size of a Node but allowing us to store more information nonetheless. BUG=595137 ========== to ========== Move LayoutObject to satellite NodeLayoutData that hangs from a Node. This patch moves the LayoutObject to be stored on LayoutData which hangs off a Node instead of being stored on the DataUnion (either directly or on a NodeRareData.) This is the first step in adding more Rendering information on a satellite object thereby not increasing the size of a Node but allowing us to store more information nonetheless. BUG=595137 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Elliott, Made the changes you asked for, PTAL? Thanks! https://codereview.chromium.org/2814603002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Node.h (right): https://codereview.chromium.org/2814603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.h:108: layout_object_ = layout_object; Done. https://codereview.chromium.org/2814603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.h:122: return node_layout_data_ ? node_layout_data_ Changed to return node_layout_data_; https://codereview.chromium.org/2814603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.h:125: void SetNodeLayoutData(NodeLayoutData* node_layout_data) { Done. https://codereview.chromium.org/2814603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.h:134: NodeLayoutData* node_layout_data_; Done. https://codereview.chromium.org/2814603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.h:594: void SetLayoutObject(LayoutObject* layout_object) { Done. https://codereview.chromium.org/2814603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.h:596: data_.rare_data_->GetNodeLayoutData() == Done. https://codereview.chromium.org/2814603002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.h:603: if (data_.node_layout_data_ == &NodeLayoutData::SharedEmptyData()) Done.
lgtm w/ the comments fixed. https://codereview.chromium.org/2814603002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/2814603002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.cpp:594: void Node::SetLayoutObject(LayoutObject* layout_object) { This allocates a NodeLayoutData when setting a nullptr for layout_object, but that's probably not what we want. You could do something like: NodeLayoutData* data = HasRareData() ? data_.rare_data_->GetNodeLayoutData() : data_.node_layout_data_; if (!data->IsSharedEmptyData()) { data->SetLayoutObject(layout_object); return; } if (!layout_object) return; data = new NodeLayoutData(layout_object); if (HasRareData()) { data_.rare_data_->SetNodeLayoutData(data); } else { data_.node_layout_data_ = data; } https://codereview.chromium.org/2814603002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Node.h (right): https://codereview.chromium.org/2814603002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.h:112: static NodeLayoutData& shared_empty_data_ = *new NodeLayoutData(nullptr); DEFINE_STATIC_LOCAL(NodeLayoutData, shared_empty_data, (nullptr)); Also no underscore at the end, it's not a member variable. https://codereview.chromium.org/2814603002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/NodeRareData.cpp (right): https://codereview.chromium.org/2814603002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/NodeRareData.cpp:80: CHECK(!GetNodeLayoutData()->GetLayoutObject()); Can we put this in the destructor of NodeLayoutData instead? ~NodeLayoutData() { CHECK(!layout_object_); } https://codereview.chromium.org/2814603002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/NodeRareData.h (right): https://codereview.chromium.org/2814603002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/NodeRareData.h:166: explicit NodeRareData(NodeLayoutData* node_layout_data) Can we add a DCHECK_NE(nullptr, node_layout_data) in the body here since it should never be null?
https://codereview.chromium.org/2814603002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/2814603002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.cpp:594: void Node::SetLayoutObject(LayoutObject* layout_object) { Done. Also commented inline. https://codereview.chromium.org/2814603002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Node.h (right): https://codereview.chromium.org/2814603002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.h:112: static NodeLayoutData& shared_empty_data_ = *new NodeLayoutData(nullptr); Done. https://codereview.chromium.org/2814603002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/NodeRareData.cpp (right): https://codereview.chromium.org/2814603002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/NodeRareData.cpp:80: CHECK(!GetNodeLayoutData()->GetLayoutObject()); Done. https://codereview.chromium.org/2814603002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/NodeRareData.h (right): https://codereview.chromium.org/2814603002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/NodeRareData.h:166: explicit NodeRareData(NodeLayoutData* node_layout_data) Done.
The CQ bit was checked by nainar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2814603002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/2814603002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.cpp:300: CHECK(HasRareData() || !GetLayoutObject()); You can remove this CHECK and the comment now, the assert is handled inside ~NodeLayoutData. https://codereview.chromium.org/2814603002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.cpp:600: // // the new LayoutObject. extra // https://codereview.chromium.org/2814603002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.cpp:609: // Swap the NodeLayoutData to point to a new NodeLayoutData instead of the // extra // https://codereview.chromium.org/2814603002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Node.h (right): https://codereview.chromium.org/2814603002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.h:106: ~NodeLayoutData() { DCHECK(!layout_object_); } I realized the old code used a CHECK for this in ~Node and ~NodeRareData, lets do that here too.
esprehn@chromium.org changed reviewers: + ikilpatrick@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Committing. Made the changes. https://codereview.chromium.org/2814603002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/2814603002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.cpp:300: CHECK(HasRareData() || !GetLayoutObject()); Done. https://codereview.chromium.org/2814603002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.cpp:600: // // the new LayoutObject. I can't even. git cl format broke this I guess. Done. https://codereview.chromium.org/2814603002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.cpp:609: // Swap the NodeLayoutData to point to a new NodeLayoutData instead of the // Done. https://codereview.chromium.org/2814603002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Node.h (right): https://codereview.chromium.org/2814603002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.h:106: ~NodeLayoutData() { DCHECK(!layout_object_); } Done.
The CQ bit was checked by nainar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org Link to the patchset: https://codereview.chromium.org/2814603002/#ps100001 (title: "esprehn comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1492122502846610,
"parent_rev": "f2a391e29205904cb61124d5bf6cfc5a13a1f8ac", "commit_rev":
"2f631c11f8e8f80c2373a6da4e4c24f7337ea55e"}
Message was sent while issue was closed.
Description was changed from ========== Move LayoutObject to satellite NodeLayoutData that hangs from a Node. This patch moves the LayoutObject to be stored on LayoutData which hangs off a Node instead of being stored on the DataUnion (either directly or on a NodeRareData.) This is the first step in adding more Rendering information on a satellite object thereby not increasing the size of a Node but allowing us to store more information nonetheless. BUG=595137 ========== to ========== Move LayoutObject to satellite NodeLayoutData that hangs from a Node. This patch moves the LayoutObject to be stored on LayoutData which hangs off a Node instead of being stored on the DataUnion (either directly or on a NodeRareData.) This is the first step in adding more Rendering information on a satellite object thereby not increasing the size of a Node but allowing us to store more information nonetheless. BUG=595137 Review-Url: https://codereview.chromium.org/2814603002 Cr-Commit-Position: refs/heads/master@{#464622} Committed: https://chromium.googlesource.com/chromium/src/+/2f631c11f8e8f80c2373a6da4e4c... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/2f631c11f8e8f80c2373a6da4e4c... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
