Closed Bug 1288590 Opened 3 years ago Closed 3 years ago

Avoid double linear search of attribute names and values

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: emilio, Assigned: emilio)

Details

Attachments

(10 files)

58 bytes, text/x-review-board-request
bholley
: review+
Details
58 bytes, text/x-review-board-request
bholley
: review+
Details
58 bytes, text/x-review-board-request
bholley
: review+
Details
58 bytes, text/x-review-board-request
bholley
: review+
Details
58 bytes, text/x-review-board-request
bholley
: review+
Details
58 bytes, text/x-review-board-request
bholley
: review+
Details
58 bytes, text/x-review-board-request
bholley
: review+
Details
58 bytes, text/x-review-board-request
bholley
: review+
Details
58 bytes, text/x-review-board-request
bholley
: review+
Details
58 bytes, text/x-review-board-request
bholley
: review+
Details
This series of patches hoist nsAttrInfo to its own header file, and uses it thorough the tree where there was a pattern like:

    const nsAttrName* name = content->GetAttrNameAt(index);
    ...
    const nsAttrValue* value = content->GetParsedAttr(name->LocalName(), name->NamespaceID());

or similar.

We could add more accessors and convenient methods to nsAttrInfo, but I've opted for keeping the struct as a really dumb container.

I've changed them in quite a few places, so I could have made some dumb mistake (hopefully not!). Try run is in progress at:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=df1607e0f425

I've tried to be careful with the semantics, and I think the only change of these that could use problems is [1], that could be called over nodes that rely on the semantics of GetAttr returning the empty string.

[1]: https://hg.mozilla.org/try/rev/062733f3776e
Heh, awesome, that try push is busted because apparently my compiler likes |operator bool() const| but the try server's don't. I'll wait to push the patches, sigh.
Review commit: https://reviewboard.mozilla.org/r/66304/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66304/
Attachment #8773885 - Flags: review?(bobbyholley)
Attachment #8773886 - Flags: review?(bobbyholley)
Attachment #8773887 - Flags: review?(bobbyholley)
Attachment #8773888 - Flags: review?(bobbyholley)
Attachment #8773889 - Flags: review?(bobbyholley)
Attachment #8773890 - Flags: review?(bobbyholley)
Attachment #8773891 - Flags: review?(bobbyholley)
Attachment #8773892 - Flags: review?(bobbyholley)
Attachment #8773893 - Flags: review?(bobbyholley)
Actual try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9741c3d04a39714c1107bca2a0b2b8641f87c26c

Seems that nothing relied on the string getting truncated in GetAttr().
So, my concern here is that nsAttrInfo is a dependent struct (i.e. it doesn't own its pointers), and any modification to the element can invalidate it. In Rust we could solve this with a lifetime, but in C++ we can't.

I've only looked at a couple of the use-cases, but it seems that the pattern of (1) Lookup the name, and then (2) lookup the value could be solved by just looking up the value by index rather than by name. Is there any reason that doesn't solve this problem>
Well, taking into account that modifying the element can also invalidate the nsAttrName pointer, I don't think this is worse.

Yes, we could look the attribute by index, but then there's the same problem, any modification to the element might change the attribute under that index, and we do another virtual function call + bounds checking.
Unfortunately couldn't add all the debug checks that I'd want, since we can't
assert that is not safe to run script in quite a few places :(

Review commit: https://reviewboard.mozilla.org/r/66568/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66568/
Attachment #8773954 - Flags: review?(bobbyholley)
Attachment #8773885 - Flags: review?(bobbyholley) → review+
Comment on attachment 8773885 [details]
Bug 1288590: Add a function to get attribute info from an Element by index.

https://reviewboard.mozilla.org/r/66304/#review63740
Attachment #8773886 - Flags: review?(bobbyholley) → review+
Comment on attachment 8773887 [details]
Bug 1288590: Use GetAttrInfoAt in nsContentUtils.cpp

https://reviewboard.mozilla.org/r/66308/#review63748
Attachment #8773887 - Flags: review?(bobbyholley) → review+
Attachment #8773888 - Flags: review?(bobbyholley) → review+
Comment on attachment 8773888 [details]
Bug 1288590: Use GetAttrInfoAt in nsXHTMLContentSerializer.cpp

https://reviewboard.mozilla.org/r/66310/#review63754
Attachment #8773889 - Flags: review?(bobbyholley) → review+
Comment on attachment 8773890 [details]
Bug 1288590: Use GetAttrInfoAt in nsXMLContentSerializer.cpp

https://reviewboard.mozilla.org/r/66314/#review63766
Attachment #8773890 - Flags: review?(bobbyholley) → review+
Attachment #8773891 - Flags: review?(bobbyholley) → review+
Comment on attachment 8773892 [details]
Bug 1288590: Use GetAttrInfoAt in nsXBLPrototypeBinding.cpp

https://reviewboard.mozilla.org/r/66318/#review63774
Attachment #8773892 - Flags: review?(bobbyholley) → review+
Attachment #8773893 - Flags: review?(bobbyholley) → review+
https://reviewboard.mozilla.org/r/66568/#review63778

r=me with those things addressed.

::: dom/base/BorrowedAttrInfo.h:24
(Diff revision 1)
> + * Struct that stores info on an attribute. The name and value must either both
> + * be null or both be non-null.
> + *
> + * Note that, just as the pointers returned by GetAttrNameAt, the pointers that
> + * this struct hold are only valid until the next call next call of either
> + * GetAttrNameAt, GetAttrInfoAt or SetAttr on the element.

Do GetAttrNameAt and GetAttrInfoAt actually invalidate the pointers? I might just say "until the element or its attributes are mutated (directly or via script)".

::: dom/base/BorrowedAttrInfo.cpp:27
(Diff revision 1)
> +#ifdef DEBUG
> +BorrowedAttrInfo::~BorrowedAttrInfo()
> +{
> +  MOZ_ASSERT_IF(mName, mValue);
> +}
> +#endif

Hm, do we really need this destructor? Seems like it doesn't do checking that the ctor doesn't do already.

::: dom/base/nsIContent.h:30
(Diff revision 1)
>  namespace mozilla {
>  class EventChainPreVisitor;
>  namespace dom {
>  class ShadowRoot;
>  struct CustomElementData;
> +struct BorrowedAttrInfo;

Given the #include, why do we need to forward-declare this?
Comment on attachment 8773954 [details]
Bug 1288590: Rename nsAttrInfo to mozilla::dom::BorrowedAttrInfo

https://reviewboard.mozilla.org/r/66568/#review63786
Attachment #8773954 - Flags: review?(bobbyholley) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc6fb539da99
Add a function to get attribute info from an Element by index. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/541dcf328482
Use GetAttrInfoAt in sdnAccessible.cpp. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab031c62a935
Use GetAttrInfoAt in nsContentUtils.cpp. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/643a88c16332
Use GetAttrInfoAt in nsXHTMLContentSerializer.cpp. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/33f5ebdb9cda
Use GetAttrInfoAt in SVGUseElement.cpp. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3ee302bec0c
Use GetAttrInfoAt in nsXMLContentSerializer.cpp. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/69c554f66ae0
Use GetAttrInfoAt in nsXBLBinding.cpp. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/11e5869e834d
Use GetAttrInfoAt in nsXBLPrototypeBinding.cpp. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/78313fb0c8f9
Use GetAttrInfoAt in ServoBindings.cpp. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/fba6db64d336
Rename nsAttrInfo to mozilla::dom::BorrowedAttrInfo. r=bholley
You need to log in before you can comment on or make changes to this bug.