Bug 193496 – bmalloc should use JSC VM tag for gigacage

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Miller]

bmalloc should use JSC VM tag for gigacage

[Keith Miller]

Created attachment 359270 [details]
Patch

[Keith Miller]

Created attachment 359273 [details]
Patch

[Mark Lam]

Comment on attachment 359273 [details]
Patch

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

r=me with fixes and if EWS bots are happy.

> Source/bmalloc/ChangeLog:9
> +        tag gigacage memory with the unused JSC memory tag. The JSC memory

I think that you should clarify that by "unused", you mean JSGigacagePages i.e. VM_TAG_FOR_GIGACAGE_MEMORY, instead of UnknownUsage.

> Source/bmalloc/bmalloc.xcodeproj/project.pbxproj:746
> +                        developmentRegion = en;

Fix indentation.

[Geoffrey Garen]

Comment on attachment 359273 [details]
Patch

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

> Source/bmalloc/ChangeLog:14
> +        * bmalloc/BVMTags.h: Copied from Source/WTF/wtf/VMTags.h.

You should just call this file "VMTags.h". The purpose of the B prefix is a cheap way, in limited cases, to avoid name conflicts with something we expect other projects to declare. But we intend to be the only declaration of VMTags.h.

> Source/bmalloc/bmalloc/BVMTags.h:73
> +    FastMallocPages = VM_TAG_FOR_TCMALLOC_MEMORY,

FastMalloc is an abstraction built on top of, and agnostically of, bmalloc. SO, it's kind of upside-down to declare the FastMalloc name inside bmalloc. I would just call this MallocPages. Also not clear why all of these names end in Pages. Also, this enum describes a tag we'll put on a piece of memory, not any constraint on its usage. So I would call it VMTag. Also, do we ever use Unknown? Seems like we don't, so let's leave it out.

[Keith Miller]

Comment on attachment 359273 [details]
Patch

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

>> Source/bmalloc/ChangeLog:9
>> +        tag gigacage memory with the unused JSC memory tag. The JSC memory
> 
> I think that you should clarify that by "unused", you mean JSGigacagePages i.e. VM_TAG_FOR_GIGACAGE_MEMORY, instead of UnknownUsage.

Geoff suggested that we get rid of the UnknownUsage. So that should clarify this concern, imo.

>> Source/bmalloc/ChangeLog:14
>> +        * bmalloc/BVMTags.h: Copied from Source/WTF/wtf/VMTags.h.
> 
> You should just call this file "VMTags.h". The purpose of the B prefix is a cheap way, in limited cases, to avoid name conflicts with something we expect other projects to declare. But we intend to be the only declaration of VMTags.h.

There's still a VMTags.h since we need to declare the macros there if not building with bmalloc. Gigacage.h being in both bmalloc and WTF already means that lldb can't debug it...

>> Source/bmalloc/bmalloc.xcodeproj/project.pbxproj:746
>> +                        developmentRegion = en;
> 
> Fix indentation.

Does it matter? This isn't a human readable file anyway, unless you're Mitz.

>> Source/bmalloc/bmalloc/BVMTags.h:73
>> +    FastMallocPages = VM_TAG_FOR_TCMALLOC_MEMORY,
> 
> FastMalloc is an abstraction built on top of, and agnostically of, bmalloc. SO, it's kind of upside-down to declare the FastMalloc name inside bmalloc. I would just call this MallocPages. Also not clear why all of these names end in Pages. Also, this enum describes a tag we'll put on a piece of memory, not any constraint on its usage. So I would call it VMTag. Also, do we ever use Unknown? Seems like we don't, so let's leave it out.

Sure. We use Unknown in OSAllocator but not here so I can get rid of it.

[Mark Lam]

Comment on attachment 359273 [details]
Patch

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

>>> Source/bmalloc/bmalloc.xcodeproj/project.pbxproj:746
>>> +                        developmentRegion = en;
>> 
>> Fix indentation.
> 
> Does it matter? This isn't a human readable file anyway, unless you're Mitz.

It matters because it's ugly ("behind the fence"). :)

>>> Source/bmalloc/bmalloc/BVMTags.h:73
>>> +    FastMallocPages = VM_TAG_FOR_TCMALLOC_MEMORY,
>> 
>> FastMalloc is an abstraction built on top of, and agnostically of, bmalloc. SO, it's kind of upside-down to declare the FastMalloc name inside bmalloc. I would just call this MallocPages. Also not clear why all of these names end in Pages. Also, this enum describes a tag we'll put on a piece of memory, not any constraint on its usage. So I would call it VMTag. Also, do we ever use Unknown? Seems like we don't, so let's leave it out.
> 
> Sure. We use Unknown in OSAllocator but not here so I can get rid of it.

nit: Since we use it in OSAllocator, I would prefer that you keep it here, rather than have these values defined in 2 places (or at least lessen it).

[Keith Miller]

Created attachment 359279 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 359279 [details]
Patch for landing

Clearing flags on attachment: 359279

Committed r240043: <https://trac.webkit.org/changeset/240043>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/47322648>

[Geoffrey Garen]

> >> Source/bmalloc/ChangeLog:14
> >> +        * bmalloc/BVMTags.h: Copied from Source/WTF/wtf/VMTags.h.
> > 
> > You should just call this file "VMTags.h". The purpose of the B prefix is a cheap way, in limited cases, to avoid name conflicts with something we expect other projects to declare. But we intend to be the only declaration of VMTags.h.
> 
> There's still a VMTags.h since we need to declare the macros there if not
> building with bmalloc. Gigacage.h being in both bmalloc and WTF already
> means that lldb can't debug it...

OK, I guess BVMTags.h is the way to go for now.

Seems like the list of duplicated things is now great enough that we would benefit from some kind of abstraction for "stuff used by bmalloc and WTF".