bmalloc should use JSC VM tag for gigacage
Created attachment 359270 [details] Patch
Created attachment 359273 [details] Patch
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.
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.
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.
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).
Created attachment 359279 [details] Patch for landing
Comment on attachment 359279 [details] Patch for landing Clearing flags on attachment: 359279 Committed r240043: <https://trac.webkit.org/changeset/240043>
All reviewed patches have been landed. Closing bug.
<rdar://problem/47322648>
> >> 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".