Closed Bug 1427028 Opened 6 years ago Closed 6 years ago

Derive FakeString from nsStringRepr

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: emk, Unassigned)

References

Details

FakeString was introduced to avoid the XPCOM string ctors/dtors cost. But nsStringRepr has been added since then that has virtually the same ctors/dtors as FakeString.

It would be better to derive it from nsStringRepr than to keep syncing the structure layout manually.
Hm, FakeString used to deliberately leave mData and mLength uninitialized, but bug 1406820 added th "missing" member initialization. Is this OK?
Flags: needinfo?(jwatt)
Flags: needinfo?(bzbarsky)
Well, I don't know.  Did the relevant microbenchmarks get rerun when those initializations were added?  That goes for NonNull as well, where we added extra initialization that didn't use to be there....

To be clear, those initializations weren't "missing"; they were left out because they were not needed and all that code is extremely performance-sensitive.
Flags: needinfo?(bzbarsky) → needinfo?(kyle)
Yeah, guessing we got overzealous on compiler warning reduction. I'll remove the initialization and add comments.
Flags: needinfo?(kyle)
Hmm, yes, I should have looked a bit deeper and skipped those parts of the flyby "fix". Or rather, I should have added the missing comments to those members to explicitly state that they're deliberately left uninitialized because the code is perf sensitive and what invariants make it safe to do that...
Flags: needinfo?(jwatt)
Thank you for the response. Then this bug is WONTFIX because FakeString would be still lighter than nsStringRepr.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.