Closed Bug 1442433 Opened 2 years ago Closed 2 years ago

Remove the refcount from static and HTML5 atoms

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: njn, Assigned: njn)

References

(Blocks 2 open bugs)

Details

Attachments

(4 files)

The refcount is only used for dynamic atoms.

On 64-bit, this reduces sizeof(nsStaticAtom) from 24 bytes to 16 bytes, and the
corresponding heap allocation from 32 bytes to 16 bytes. This saves 42 KiB per
process.

On 32-bit, this reduces sizeof(nsStaticAtom) from 16 bytes to 12 bytes, but the
corresponding heap allocated stays at 16 bytes, so memory usage is unchanged.

The change will also reduce the size of HTML5 atoms, but they are short-lived
so the difference won't be noticeable.

MozReview-Commit-ID: 7d9H7MRHN9a
Attachment #8955334 - Flags: review?(nfroyd)
Also remove the unused SetKind() method.

MozReview-Commit-ID: CIh6BmN7OLI
Attachment #8955409 - Flags: review?(nfroyd)
Comment on attachment 8955334 [details] [diff] [review]
Remove the refcount from static atoms

Review of attachment 8955334 [details] [diff] [review]:
-----------------------------------------------------------------

So easy.
Attachment #8955334 - Flags: review?(nfroyd) → review+
Comment on attachment 8955409 [details] [diff] [review]
Make some nsAtom fields `const`

Review of attachment 8955409 [details] [diff] [review]:
-----------------------------------------------------------------

\o/
Attachment #8955409 - Flags: review?(nfroyd) → review+
The patch also uses GetStringBuffer() in a couple of appropriate places.

MozReview-Commit-ID: JufCUgmO8JL
Attachment #8955951 - Flags: review?(nfroyd)
I'm a bit ambivalent about this one. What do you think?
Attachment #8955967 - Flags: review?(nfroyd)
Attachment #8955951 - Flags: review?(nfroyd) → review+
Comment on attachment 8955967 [details] [diff] [review]
Make nsAtom::mString even more const

Review of attachment 8955967 [details] [diff] [review]:
-----------------------------------------------------------------

I think the idea is reasonable, even if the const_cast is ugly.

I wonder if we could push the constness farther, by:

1) making nsStringBuffer::FromData take `const void*`.  We'd still have to const_cast, but I think it's a little more justified inside FromData than elsewhere; and/or
2) making nsStringBuffer more const itself--mStorageSize should be const, as should the canary (I think).  We'd have to mark the refcnt `mutable`, but I think that's OK (see ConstRemovingRefPtrTraits in RefPtr.h for a discussion of the issues).

Both of those (especially #2) are followups, of course.

I think the patch improves the state of the world.
Attachment #8955967 - Flags: review?(nfroyd) → review+
You need to log in before you can comment on or make changes to this bug.