Kinda feel like these should have the characters8/16 name long term and the raw pointers one can either go away or have a longer name. Not worth changing in this patch, but maybe worth a comment.

I tried to be consistent with StringView which already had those:

    const LChar* characters8() const;
    const UChar* characters16() const;
    Span<const LChar> span8() const { return { characters8(), length() }; }
    Span<const UChar> span16() const { return { characters16(), length() }; }

For future consideration, I like Sam’s idea even though I am the one who named the functions in StringView.

Did you consider using an abstraction like the StringParsingBuffer rather than raw pointers?

This is a straight import. I didn't make any tweaks yet. I think we should adopt more WebKit concepts, though I was hoping to do it in a follow-up since each change will have to be perf-tested.

I'll give it a shot in a follow-up and perf test it. I would be nicer than dealing with pointers indeed.

I was able to rewrite the code using StringParsingBuffer easily. I am waiting on A/B perf results.

Looks like a small regression at the moment, I am trying to figure out why.

For future consideration, I like Sam’s idea even though I am the one who named the functions in StringView.

Could this all go into the fragment->parseHTML function, not here? The abstraction is "faster way to parse", not "two different ways to parse", so I think one level deeper.

The problem is that the fast path can start parsing and populating the fragment, then encounter something it can't parse and give up. When it gives up we currently destroy the fragment and reconstruct a new one before calling fragment->parseHTML(). This reconstruction makes what you suggest not possible so we'd need another approach. I already tried calling removeChildren() on the fragment but that turned out to be too expensive. I need to find a cheaper way.

What is this change for? Change log doesn’t say.

Same as above.

What is this for? Change log doesn’t say.

I'll add in the commit log. A while back, we landed a Speedometer optimization in HTMLInputElement which delays initialization of the inputType when constructed by the parser. This was to avoid creating an inputType for "text" only to figure out later during parsing that we wanted another type.

There is a case in the new HTML fast parsing case where we give up parsing a destroy a newly constructed HTMLInputElement before we're done fully parsing it. As a result, the input element may now get destroyed before its m_inputType got initialized. However, the HTMLInputElement would previously crash if m_inputType is null. It seemed like a waste to force the initialization of m_inputType in this case so I decided to go the other way and add support for an HTMLInputElement to get destroyed before its m_inputType gets initialized.

We should leave this MaxValue thing out since it’s never used.

Please use the name character rather than c.

OK.

Is it because it is a UChar32?

You didn't make the same comment for the other functions taking a c so I started wondering.

I just ran out of steam reviewing, really. I think occasionally a single letter name could be justified but in a short function I think we’d want to used a word instead of a letter.

This seems inefficient. Why not just have appendLegalEntityFor take a Vector<UChar>? Making and destroying a string each time won’t be fast.

Oh good point! I'll make this change. I only used StringBuilder for consistency with the other API in that header but didn't think enough about the performance implications.

Planning on going with:

void appendLegalEntityFor(UChar32 c, Vector<UChar>& decodedEntity)
{
    auto legalEntity = HTMLEntityParser::legalEntityFor(c);
    if (U_IS_BMP(legalEntity)) {
        decodedEntity.append(static_cast<UChar>(legalEntity));
        return;
    }
    decodedEntity.append(U16_LEAD(legalEntity));
    decodedEntity.append(U16_TRAIL(legalEntity));
}

The name decodedEntity is not a good name for a StringBuilder that we will be appending to. Sure, the thing we are appending is a decoded entity.

Ok. Proposing ouputBuffer

These argument names: result and res, not very helpful. I suggest returnValue instead of res.

Will use ResultType -> ReturnValueType for the template parameter as well, to avoid confusion.

Is sorting really the fastest way to find duplicates? Seems unlikely. I think a HashSet<AtomString> would be faster.

Ok. I'll add a FIXME comment for now and evaluate this in a follow-up.

Well, interestingly, switching to a HashSet appears to be a confirmed 0.8% regression on Speedometer on iOS.
My bet at the moment is that the issue with the HashSet is that it we don't have a way to clear it while retaining its internal capacity. Unlike a Vector where you can resize(0) and drop its items while retaining its capacity. This is helpful since the parser currently reuses the same parser between elements.

I think we have varieties of HashSet including ones with minimum capacity so we don't spend so much time reallocating memory. If the regression really about the memory allocation and not the hashing I think we can fix it. But maybe this is already fast enough.

But I guess all of them lack a clear function that clears in place. I do think a version that only grows and never shrinks could be useful in many places.

I am A/B testing a version with a HashSet that doesn't clear capacity so see if it is faster. If not, I guess the cost would be the hashing.

So the version that uses a HashSet and clears it while maintaining its capacity is perf-neutral. It's a little disappointing.
We may still want to do it since a HashSet would be less prone to pathological cases than vector sorting & deduping.

FYI, this vector is a big contributor to fastMalloc() in the profiles. I think ideally we'd use Vector<Attribute, 10> like we do in the full HTML parser. However, this requires some code changes to make it build/work. I am currently A/B testing such change.