Bug 111250 – AtomicHTMLToken should not be heap allocated or RefCounted

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

AtomicHTMLToken should not be heap allocated or RefCounted

This is done so that it can be saved on the HTMLElementStack, but it appears we only use this in 3 places:

/Users/eseidel/Projects/WebKit/Source/WebCore/html/parser/HTMLElementStack.cpp:
  291          return false;
  292      if (item->hasTagName(MathMLNames::annotation_xmlTag)) {
  293:         Attribute* encodingAttr = item->token()->getAttributeItem(MathMLNames::encodingAttr);
  294          if (encodingAttr) {
  295              const String& encoding = encodingAttr->value();

/Users/eseidel/Projects/WebKit/Source/WebCore/html/parser/HTMLFormattingElementList.cpp:
  179      remainingCandidates.reserveInitialCapacity(candidates.size());
  180  
  181:     const Vector<Attribute>& attributes = newItem->token()->attributes();
  182      for (size_t i = 0; i < attributes.size(); ++i) {
  183          const Attribute& attribute = attributes[i];
  ...
  190              ASSERT(newItem->localName() == candidate->localName() && newItem->namespaceURI() == candidate->namespaceURI());
  191  
  192:             Attribute* candidateAttribute = candidate->token()->getAttributeItem(attribute.name());
  193              if (candidateAttribute && candidateAttribute->value() == attribute.value())
  194                  remainingCandidates.append(candidate);

This just adds an extra malloc, and some complexity to AtomicHTMLToken we don't need, including the ability to clear it's externalCharacterBuffer pointers so we don't grab at memory after its freed.  See bug 111248.

[Eric Seidel (no email)]

I believe this is the fastMalloc showing up as .6% of time profiling Parser/html-parser-threaded.html with bug 107236 applied:

Running Time	Self		Symbol Name
108.5ms    6.2%	108.5	 	WTF::fastMalloc(unsigned long)
12.1ms    0.6%	0.0	 	 WebCore::HTMLDocumentParser::processParsedChunkFromBackgroundParser(WTF::PassOwnPtr<WebCore::HTMLDocumentParser::ParsedChunk>)

[Eric Seidel (no email)]

Before:

RESULT Parser: html-parser-threaded: Time= 450.164849998 ms
median= 450.482999993 ms, stdev= 7.2381436538 ms, min= 436.084999994 ms, max= 464.968999964 ms
RESULT Parser: html-parser-threaded: JSHeap= 3522054.8 bytes
median= 3603242.0 bytes, stdev= 623535.479063 bytes, min= 2623588.0 bytes, max= 4378336.0 bytes
RESULT Parser: html-parser-threaded: Malloc= 53670691.6 bytes
median= 56351944.0 bytes, stdev= 11597207.0499 bytes, min= 36349272.0 bytes, max= 69981056.0 bytes

After:

RESULT Parser: html-parser-threaded: Time= 436.115300003 ms
median= 436.41600004 ms, stdev= 5.71435647554 ms, min= 427.160999971 ms, max= 446.753000026 ms
RESULT Parser: html-parser-threaded: JSHeap= 3548257.8 bytes
median= 3603324.0 bytes, stdev= 635649.475906 bytes, min= 2623588.0 bytes, max= 4509212.0 bytes
RESULT Parser: html-parser-threaded: Malloc= 54642561.6 bytes
median= 56337700.0 bytes, stdev= 11804768.9883 bytes, min= 36077416.0 bytes, max= 69851960.0 bytes

I makes it faster.

[Eric Seidel (no email)]

Created attachment 192173 [details]
Remove infinity mallocs from parsing

[Eric Seidel (no email)]

Comment on attachment 192173 [details]
Remove infinity mallocs from parsing

View in context: https://bugs.webkit.org/attachment.cgi?id=192173&action=review

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:542
> +    // NOTE: Moving from item -> token -> item copies the Attribute vector twice!

These mallocs do show up on the profile.  But there are *way* fewer of them than all the AtomicHTMLTokens we were mallocing before. :)

> Source/WebCore/html/parser/HTMLStackItem.h:-211
> -            // Create a fake token for a document fragment node. This looks ugly but required for performance
> -            // because we want to use m_token->name() in localName(), hasLocalName() and hasTagName() without
> -            // checking m_isDocumentFragmentNode flag.
> -            m_token = AtomicHTMLToken::create(HTMLToken::StartTag, nullAtom);

It's possible I'm subtly changing behavior here... or that this code wasn't actually doing what it thought it was doing.  The new code should still return nullAtom from localName() like the old code did. :)

[Adam Barth]

Comment on attachment 192173 [details]
Remove infinity mallocs from parsing

View in context: https://bugs.webkit.org/attachment.cgi?id=192173&action=review

> Source/WebCore/html/parser/HTMLStackItem.h:227
> +        , m_tokenAttributes(token->attributes())

If we were clever, we could probably swap the attributes in and out of this Vector.

> Source/WebCore/html/parser/HTMLStackItem.h:237
> +    AtomicString m_tokenLocalName;
> +    Vector<Attribute> m_tokenAttributes;
>      AtomicString m_namespaceURI;

The names m_tokenLocalName and m_namespaceURI aren't quite symmetric.  Maybe they're different subtly?

[Eric Seidel (no email)]

Comment on attachment 192173 [details]
Remove infinity mallocs from parsing

View in context: https://bugs.webkit.org/attachment.cgi?id=192173&action=review

>> Source/WebCore/html/parser/HTMLStackItem.h:227
>> +        , m_tokenAttributes(token->attributes())
> 
> If we were clever, we could probably swap the attributes in and out of this Vector.

Definitely something to explore. Will require changing the api contract with the token.

>> Source/WebCore/html/parser/HTMLStackItem.h:237
>>      AtomicString m_namespaceURI;
> 
> The names m_tokenLocalName and m_namespaceURI aren't quite symmetric.  Maybe they're different subtly?

Namespace comes from the context.  Token is only set for formatting elements iirc.

[Eric Seidel (no email)]

Comment on attachment 192173 [details]
Remove infinity mallocs from parsing

Thanks!

[WebKit Review Bot]

Comment on attachment 192173 [details]
Remove infinity mallocs from parsing

Clearing flags on attachment: 192173

Committed r145248: <http://trac.webkit.org/changeset/145248>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.