1470365 - Use shared memory for string bundles loaded in content processes

Status: RESOLVED FIXED

(Core :: Internationalization, enhancement, P3)

Description

[Kris Maglione [:kmag]]

+++ This bug was initially created as a clone of Bug #1451568 +++

We should be able to load string bundles needed by content processes in the parent, and then share a compact, read-only representation with child processes.

I have a prototype of this already, so I'll take the bug.

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

I would be interested in using similar approach for L10nRegistry and FTL files at some point.

Comment 2

[Kris Maglione [:kmag]]

From just about:memory reports, it looks like this saves at least 160K per content process in just reported memory. It probably saves more in unreported memory.

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1470365: Part 1 - Add a compact, read-only, shared-memory string map class.

This class implements a shared memory key-value store that fits into a single
memory mapped segment. All of the runtime data for its instances are stored in
the shared memory region, which means that memory overhead for each instance
in each process is only a few bytes.

Importantly, the key and value strings returned by this class are also
pointers into the shared memory region, which means that once an instance is
created, its memory cannot be unmapped until process shutdown.

For the uses I intend to put it to, this is a reasonable constraint. If we
need to use it for shorter-lived maps in the future, we can add an option to
return non-literal dependent strings that will be copied if they need to be
kept alive long term.

Review commit: https://reviewboard.mozilla.org/r/252568/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/252568/

Comment 4

[Kris Maglione [:kmag]]

Attachment: Bug 1470365: Part 2 - Add StringBundle implementation using shared memory maps.

This adds a basic StringBundle implementation using shared memory maps, but
doesn't yet hook them up to anything. They behave the same as ordinary string
bundles, but have the additional restriction that they are permanent for the
length of the session, and cannot be removed from the cache.

Review commit: https://reviewboard.mozilla.org/r/252570/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/252570/

Comment 5

[Kris Maglione [:kmag]]

Attachment: Bug 1470365: Part 3 - Use shared memory for StringBundles loaded in the content process.

Review commit: https://reviewboard.mozilla.org/r/252572/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/252572/

Comment 6

[Kris Maglione [:kmag]]

This also has some content process startup perf wins, since it saves us from having to load and process thousands of property file entries at startup for each content process.

Comment 7

[Eric Rahm [:erahm]]

Comment on attachment 8987320 [details]
Bug 1470365: Part 1 - Add a compact, read-only, shared-memory string map class.

https://reviewboard.mozilla.org/r/252568/#review260446

Looks good, just some minor comments/requests.

::: dom/ipc/SharedStringMap.h:39
(Diff revision 1)
> +class SharedStringMap
> +{
> +  using FileDescriptor = mozilla::ipc::FileDescriptor;
> +
> +public:
> +  struct Layout {

Can you document the actual layout in shmem?
`Layout` itself is a bit of an overloaded term, maybe just `Header`?

It would be a good idea to encode the sizes of the key and value sections so that we can check for valid accesses in the StringTable class and in general that we get a valid size when encoding/decoding the shmem.

::: dom/ipc/SharedStringMap.h:41
(Diff revision 1)
> +  using FileDescriptor = mozilla::ipc::FileDescriptor;
> +
> +public:
> +  struct Layout {
> +    uint32_t mEntryCount;
> +    size_t mKeyStrings;

Maybe `uintptr_t` for these?

::: dom/ipc/SharedStringMap.h:41
(Diff revision 1)
> +  using FileDescriptor = mozilla::ipc::FileDescriptor;
> +
> +public:
> +  struct Layout {
> +    uint32_t mEntryCount;
> +    size_t mKeyStrings;

Docs for these as well. Is this an offset? An actual size?

::: dom/ipc/SharedStringMap.h:45
(Diff revision 1)
> +    uint32_t mEntryCount;
> +    size_t mKeyStrings;
> +    size_t mValueStrings;
> +  };
> +
> +  struct StringEntry {

Can you doc this as well? Where is the offset from?

::: dom/ipc/SharedStringMap.h:128
(Diff revision 1)
> +
> +private:
> +  template <typename StringType>
> +  class StringTable
> +  {
> +    using ElemType = decltype(DeclVal<StringType>()[0]);

StringType::char_type might be clearer for our use cases.

::: dom/ipc/SharedStringMap.h:131
(Diff revision 1)
> +  class StringTable
> +  {
> +    using ElemType = decltype(DeclVal<StringType>()[0]);
> +
> +  public:
> +    MOZ_IMPLICIT StringTable(char* aBuffer)

Perhaps `uint8_t*` or just `void*` would make more sense. Or really `ElemType*` and push the casting to the caller. 

Using a `Span` might be safer; I was going suggest adding asserts that the offset is valid in `GetBare` but we don't know the size. `Span` will give us those for free.

::: dom/ipc/SharedStringMap.h:142
(Diff revision 1)
> +
> +    StringType Get(const StringEntry& aEntry) const
> +    {
> +      StringType res;
> +      res.AssignLiteral(GetBare(aEntry), aEntry.mLength);
> +      return std::move(res);

This might trip up copy elision warnings in more recent compilers. Just a standard return should probably suffice, although I don't know what our policy is for that as far as trusting our tier 1 compilers to do the right thing.

::: dom/ipc/SharedStringMap.h:147
(Diff revision 1)
> +      return std::move(res);
> +    }
> +
> +    const ElemType* GetBare(const StringEntry& aEntry) const
> +    {
> +      return reinterpret_cast<ElemType*>(&mBuffer[aEntry.mOffset]);

This cast isn't necessary.

::: dom/ipc/SharedStringMap.h:209
(Diff revision 1)
> +   */
> +  Result<Ok, nsresult> Finalize(loader::AutoMemMap& aMap);
> +
> +private:
> +  template <typename KeyType, typename StringType>
> +  class StringTable

I don't have a great solution here, but having second StringTable (and Entry) definition in the same file is a bit confusing.

::: dom/ipc/SharedStringMap.h:219
(Diff revision 1)
> +    using ElemType = decltype(DeclVal<StringType>()[0]);
> +
> +    uint32_t Add(const StringType& aKey)
> +    {
> +      Entry& entry = mEntries.GetOrInsert(aKey);
> +      if (entry.mOffset == SENTINEL) {

You can get rid of `SENTINEL` and use [LookupForAdd](https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/xpcom/ds/nsBaseHashtable.h#332-362) to detect if an entry was present or not instead.

::: dom/ipc/SharedStringMap.cpp:60
(Diff revision 1)
> +
> +bool
> +SharedStringMap::Has(const nsCString& aKey)
> +{
> +  nsString value;
> +  return Get(aKey, value);

Can you split out the index lookup and share that here to avoid the dummy value and extra assignment?

::: dom/ipc/SharedStringMap.cpp:94
(Diff revision 1)
> +
> +Result<Ok, nsresult>
> +SharedStringMapBuilder::Finalize(loader::AutoMemMap& aMap)
> +{
> +  using Layout = SharedStringMap::Layout;
> +

Maybe assert that keys, vals, and entries are all the same length.

::: js/xpconnect/loader/AutoMemMap.cpp:152
(Diff revision 1)
>  
>  void
>  AutoMemMap::reset()
>  {
>      if (fileMap) {
> -        if (addr) {
> +        if (addr && !persistent_) {

I wonder if we should transfer the mapping to some sort of shutdown listener.

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8987320 [details]
Bug 1470365: Part 1 - Add a compact, read-only, shared-memory string map class.

https://reviewboard.mozilla.org/r/252568/#review260446

> Maybe `uintptr_t` for these?

They're offsets, so they should be `size_t` or `off_t`, but `off_t` is probably not available on Windows.

> StringType::char_type might be clearer for our use cases.

Yeah, probably. I didn't realize that was public.

> Perhaps `uint8_t*` or just `void*` would make more sense. Or really `ElemType*` and push the casting to the caller. 
> 
> Using a `Span` might be safer; I was going suggest adding asserts that the offset is valid in `GetBare` but we don't know the size. `Span` will give us those for free.

There was a reason I switched to `char*` from `uint8_t*`, but I don't remember what it was. I think there was some constructor that wanted `char*` and I didn't want the extra casts. But `uint8_t*` probably makes more sense.

`void*` isn't great, since this is a binary buffer that we need indexed offsets into.

If we're going to go the bounds-checking route, I'd rather use `RangedPtr` so we get the bounds checking automatically in debug builds but don't pass unused sizes around elsewhere. That's what we do for the pointers returned by AutoMemMap.

> This might trip up copy elision warnings in more recent compilers. Just a standard return should probably suffice, although I don't know what our policy is for that as far as trusting our tier 1 compilers to do the right thing.

It doesn't. I got those warnings some other places I tried to do that, but not here. I'm not entirely sure why. Either way, it's probably safe to remove the `std::move` here.

> I don't have a great solution here, but having second StringTable (and Entry) definition in the same file is a bit confusing.

Yeah, it's been getting on my nerves, too. I was planning to rename this one `StringTableBuilder`

> I wonder if we should transfer the mapping to some sort of shutdown listener.

Maybe. I initially thought about that, but all of the ClearOnShutdown hooks run pretty early, and I didn't want to try to guarantee that all of the references were dead by then... I guess we could add some later hook, though.

Comment 9

[Eric Rahm [:erahm]]

Comment on attachment 8987321 [details]
Bug 1470365: Part 2 - Add StringBundle implementation using shared memory maps.

https://reviewboard.mozilla.org/r/252570/#review260484

Mostly minor stuff, the main issue for me is whether retargeting the proxy needs to be thread safe or not.

::: intl/strres/nsStringBundle.cpp:56
(Diff revision 1)
> -NS_IMPL_ISUPPORTS(nsStringBundle, nsIStringBundle)
> +/**
> + * A set of string bundle URLs which are loaded by content processes, and
> + * should be allocated in a shared memory region, and then sent to content
> + * processes.
> + */
> +static char kContentBundles[][52] = {

It's not clear what's going on with that 52, but I think we really want:

`static const char* const kContentBundles[] = {`

More generally, how do we make sure this gets updated when bundles are added or removed? Maybe add a test?

::: intl/strres/nsStringBundle.cpp:77
(Diff revision 1)
> +  "chrome://necko/locale/necko.properties",
> +  "chrome://onboarding/locale/onboarding.properties",
> +};
>  
> -nsStringBundle::nsStringBundle(const char* aURLSpec,
> +static bool
> +IsContentBundle(const nsCString& aUrl)

nit: `const nsACString&`

::: intl/strres/nsStringBundle.cpp:114
(Diff revision 1)
> +
> +  explicit StringBundleProxy(already_AddRefed<nsIStringBundle> aTarget)
> +    : mTarget(aTarget)
> +  {}
> +
> +  NS_FORWARD_NSISTRINGBUNDLE(mTarget->);

This doesn't seem threadsafe given `Retarget` below.

::: intl/strres/nsStringBundle.cpp:129
(Diff revision 1)
> +  }
> +
> +  size_t SizeOfIncludingThisIfUnshared(mozilla::MallocSizeOf aMallocSizeOf) const override
> +  {
> +    size_t size = mTarget->SizeOfIncludingThisIfUnshared(aMallocSizeOf);
> +    if (mRefCnt > 1) {

This should be `== 1`

::: intl/strres/nsStringBundle.cpp:169
(Diff revision 1)
> +  {}
> +
> +  /**
> +   * Initialize the string bundle with a file descriptor pointing to a
> +   * pre-populated key-value store for this string bundle. This should only be
> +   * called in child processes, for bundles initially created in the parent

Can we assert that?

::: intl/strres/nsStringBundle.cpp:182
(Diff revision 1)
> +  nsresult LoadProperties() override;
> +
> +  /**
> +   * Returns a copy of the file descriptor pointing to the shared memory
> +   * key-values tore for this string bundle. This should only be called in the
> +   * parent process, and may be used to send shared string bundles to child

Can we assert that?

::: intl/strres/nsStringBundle.cpp:481
(Diff revision 1)
>                                               nsDependentCString(aName),
>                                               aResult);
>      if (NS_SUCCEEDED(rv)) return rv;
>    }
>  
> -  return mProps->GetStringProperty(nsDependentCString(aName), aResult);
> +  return GetStringImpl(nsDependentCString(aName), aResult);

This moves the scope of `LoadProperties` to be covered by `mReentrantMonitor`. Is that what we want?

Comment 10

[Eric Rahm [:erahm]]

Comment on attachment 8987321 [details]
Bug 1470365: Part 2 - Add StringBundle implementation using shared memory maps.

https://reviewboard.mozilla.org/r/252570/#review260484

Oh also if there's an appropriate peer for this code I think they should be tagged in.

Comment 11

[Eric Rahm [:erahm]]

Comment on attachment 8987321 [details]
Bug 1470365: Part 2 - Add StringBundle implementation using shared memory maps.

https://reviewboard.mozilla.org/r/252570/#review260522

::: intl/strres/nsStringBundle.cpp:992
(Diff revision 1)
>  
> +    shared = do_QueryObject(cacheEntry->mBundle);
>    } else {
>      // hasn't been cached, so insert it into the hash table
> -    RefPtr<nsStringBundle> bundle = new nsStringBundle(aURLSpec, mOverrideStrings);
> +    nsCOMPtr<nsIStringBundle> bundle;
> +    bool isContent = IsContentBundle(PromiseFlatCString(key));

You shouldn't need `PromiseFlatCString` here anymore.

::: intl/strres/nsStringBundleService.h:69
(Diff revision 1)
>    bundleCacheEntry_t *insertIntoCache(already_AddRefed<nsIStringBundle> aBundle,
> -                                      nsCString &aHashKey);
> +                                      const nsCString &aHashKey);
>  
>    nsDataHashtable<nsCStringHashKey, bundleCacheEntry_t*> mBundleMap;
>    mozilla::LinkedList<bundleCacheEntry_t> mBundleCache;
> +  mozilla::LinkedList<bundleCacheEntry_t> mSharedBundles;

This needs to be dealt with in the destructor.

Comment 12

[Kris Maglione [:kmag]]

Comment on attachment 8987321 [details]
Bug 1470365: Part 2 - Add StringBundle implementation using shared memory maps.

(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #10)
> Oh also if there's an appropriate peer for this code I think they should be
> tagged in.

As far as the implementation details go, I think this code is more or less un-owned at this point. I've been flagging gandalf for patches that remove unused functionality, though. And as he's expressed interest in doing something similar elsewhere, I suppose his review could not hurt.

Comment 13

[Eric Rahm [:erahm]]

Comment on attachment 8987322 [details]
Bug 1470365: Part 3 - Use shared memory for StringBundles loaded in the content process.

https://reviewboard.mozilla.org/r/252572/#review260526

This seems okay, but I'd like to have a dom peer look at it as well.

::: intl/strres/nsStringBundle.cpp:1019
(Diff revision 1)
> +  if (cacheEntry) {
> +    if (RefPtr<SharedStringBundle> shared = do_QueryObject(cacheEntry->mBundle)) {
> +      return;
> +    }
> +
> +    proxy = do_QueryObject(cacheEntry->mBundle);

Maybe assert proxy?

::: intl/strres/nsStringBundle.cpp:1030
(Diff revision 1)
> +
> +  if (proxy) {
> +    proxy->Retarget(bundle);
> +  }
> +
> +  cacheEntry = insertIntoCache(bundle.forget(), nsCString(aBundleURL));

Not your bug, but maybe make insertIntoCache take a `const nsACString&`?

Comment 14

[Kris Maglione [:kmag]]

Comment on attachment 8987321 [details]
Bug 1470365: Part 2 - Add StringBundle implementation using shared memory maps.

https://reviewboard.mozilla.org/r/252570/#review260484

> It's not clear what's going on with that 52, but I think we really want:
> 
> `static const char* const kContentBundles[] = {`
> 
> More generally, how do we make sure this gets updated when bundles are added or removed? Maybe add a test?

I actually mostly cribbed this layout from nsContentUtils.cpp, which does the same thing. The weird double layout is to avoid relocations for the `char*` pointers. 52 is the length of the longest string, plus null terminator. The others get a bit of extra padding, but on 64 bit systems, that extra padding is less than the size of the extra array of `char*` pointers we'd get if we used a different layout, so we save memory this way even ignoring relocations.

A test would probably be a good idea, yeah...

> This doesn't seem threadsafe given `Retarget` below.

Hm. Yeah, I suppose I should try to at least make the retargeting atomic...

> Can we assert that?

Yes. I did add assertions in a few places. There might be some more I can add, though.

> This moves the scope of `LoadProperties` to be covered by `mReentrantMonitor`. Is that what we want?

Yes. Decidedly. I was actually kind of horrified when I first read this...

I think, right now, this only works if LoadProperties is called on the main thread, before the bundle is ever accessed off-main-thread. We should probably also add an assertion that we never try to actually load properties off-main-thread, because the network stuff definitely isn't thread-safe.

But either way, the checks for whether we're initialized should happen inside the reentrant monitor.

Comment 15

[Kris Maglione [:kmag]]

Comment on attachment 8987321 [details]
Bug 1470365: Part 2 - Add StringBundle implementation using shared memory maps.

https://reviewboard.mozilla.org/r/252570/#review260522

> This needs to be dealt with in the destructor.

Yeah, I realized that after I pushed and changed it to an AutoCleanLinkedList. I thought I'd updated the reviews after that.

Comment 16

[Kris Maglione [:kmag]]

Comment on attachment 8987321 [details]
Bug 1470365: Part 2 - Add StringBundle implementation using shared memory maps.

https://reviewboard.mozilla.org/r/252570/#review260484

> nit: `const nsACString&`

Alas, that doesn't work. I tried it at first, but nsACString doesn't have a `Compare` method.

Comment 17

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8987321 [details]
Bug 1470365: Part 2 - Add StringBundle implementation using shared memory maps.

https://reviewboard.mozilla.org/r/252570/#review260546

::: intl/strres/nsStringBundle.cpp:56
(Diff revision 1)
> -NS_IMPL_ISUPPORTS(nsStringBundle, nsIStringBundle)
> +/**
> + * A set of string bundle URLs which are loaded by content processes, and
> + * should be allocated in a shared memory region, and then sent to content
> + * processes.
> + */
> +static char kContentBundles[][52] = {

Can we lock down that you can't add a content process .properties file without adding it to this list?
If that's not possible because of the short-lived bundles, can we alter the API to require to pass a flag to retrieve a bundle in the content process that is not shared? I'd like to start making the scenario of fetching a short-lived bundle in content process greppable.

::: intl/strres/nsStringBundle.cpp:960
(Diff revision 1)
>    while (!mBundleCache.isEmpty()) {
>      delete mBundleCache.popFirst();
>    }
> +
> +  // We never flush shared bundles, since their memory cannot be freed, so add
> +  // them back to the map.

What happens when you change language at runtime here? We flush string bundles and expect to get them refetched/parsed on the next call.

Comment 18

[Kris Maglione [:kmag]]

Comment on attachment 8987321 [details]
Bug 1470365: Part 2 - Add StringBundle implementation using shared memory maps.

https://reviewboard.mozilla.org/r/252570/#review260546

> Can we lock down that you can't add a content process .properties file without adding it to this list?
> If that's not possible because of the short-lived bundles, can we alter the API to require to pass a flag to retrieve a bundle in the content process that is not shared? I'd like to start making the scenario of fetching a short-lived bundle in content process greppable.

We could, but some bundles are only used in some content processes, some of the time, so it's going to be hard to make sure that doesn't break any corner cases.

Also, adding a bundle to this list only really helps things if we load it in the parent process, so child processes actually receive snapshots, and I want to add better infrastructure for that in a follow-up before we start requiring its use.

> What happens when you change language at runtime here? We flush string bundles and expect to get them refetched/parsed on the next call.

These string bundles are cached by nsContentUtils and never flushed, regardless of locale switches, so this is more or less unsupported at the moment.

I did consider adding special handling for locale switch flushes, but given how poorly that's supported at the moment (and that it wouldn't affect most uses of these bundles anyway), it didn't seem worth it.

It might be worth doing in a follow-up when we improve our support for runtime locale switches, though.

Comment 19

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8987321 [details]
Bug 1470365: Part 2 - Add StringBundle implementation using shared memory maps.

Thanks! That sounds good to me.

While I'd love to move all StringBundles to Fluent which does support live switching, the initial runtime switching will induce a restart because how much of the code is still not ready. Especially all those cases with double and triple caching of StringBundles.

Please, file a follow up tho, so that we don't regress in places where we already react well to language switching ("app-locales-changed" event).

Comment 20

[Kris Maglione [:kmag]]

Comment on attachment 8987320 [details]
Bug 1470365: Part 1 - Add a compact, read-only, shared-memory string map class.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252568/diff/1-2/

Comment 21

[Kris Maglione [:kmag]]

Comment on attachment 8987321 [details]
Bug 1470365: Part 2 - Add StringBundle implementation using shared memory maps.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252570/diff/1-2/

Comment 22

[Kris Maglione [:kmag]]

Comment on attachment 8987322 [details]
Bug 1470365: Part 3 - Use shared memory for StringBundles loaded in the content process.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252572/diff/1-2/

Comment 23

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8987321 [details]
Bug 1470365: Part 2 - Add StringBundle implementation using shared memory maps.

https://reviewboard.mozilla.org/r/252570/#review260562

Comment 24

[Eric Rahm [:erahm]]

Comment on attachment 8987321 [details]
Bug 1470365: Part 2 - Add StringBundle implementation using shared memory maps.

https://reviewboard.mozilla.org/r/252570/#review260570

Looks good, just one minor change requested.

::: intl/strres/nsStringBundle.cpp:56
(Diff revisions 1 - 2)
>  /**
>   * A set of string bundle URLs which are loaded by content processes, and
>   * should be allocated in a shared memory region, and then sent to content
>   * processes.
> + *
> + * Note: This layout is chosen to avoid having to create a separate char*

Thanks for the thorough comment! It should still be marked const or it'll end up in .data.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8987322 [details]
Bug 1470365: Part 3 - Use shared memory for StringBundles loaded in the content process.

https://reviewboard.mozilla.org/r/252572/#review260564

::: dom/base/nsContentUtils.cpp:3931
(Diff revision 2)
>  {
> +  // We only ever want to pre-create bundles in the parent process.
> +  //
> +  // All nsContentUtils bundles are shared between the parent and child
> +  // precesses, and the shared memory regions that back them *must* be created
> +  // in the parent, and then sent to all children.

(not native English speaker but should it be "all the children"?)

::: intl/strres/nsStringBundle.cpp:1041
(Diff revision 2)
> +                                             const FileDescriptor& aMapFile,
> +                                             size_t aMapSize)
> +{
> +  RefPtr<StringBundleProxy> proxy;
> +
> +  auto* cacheEntry = mBundleMap.Get(aBundleURL);

Don't use auto here. It isn't clear to the reader what the type is.

::: intl/strres/nsStringBundle.cpp:1052
(Diff revision 2)
> +    proxy = do_QueryObject(cacheEntry->mBundle);
> +    MOZ_ASSERT(proxy);
> +    cacheEntry->remove();
> +  }
> +
> +  auto bundle = MakeRefPtr<SharedStringBundle>(aBundleURL.get(), mOverrideStrings);

I'd rather not use MakeRefPtr.
This would be easier to read if this was
RefPtr<SharedStringBundle> bundle = new
SharedStringBundle(aBundleURL.get(), mOverrideStrings);

::: intl/strres/nsStringBundleService.h:71
(Diff revision 2)
>                              nsAString& result);
>  
>    void flushBundleCache();
>  
> -  bundleCacheEntry_t *insertIntoCache(already_AddRefed<nsIStringBundle> aBundle,
> -                                      const nsCString &aHashKey);
> +  bundleCacheEntry_t* insertIntoCache(already_AddRefed<nsIStringBundle> aBundle,
> +                                      const nsACString &aHashKey);

want to move & to right place, next to the type.

Comment 26

[Eric Rahm [:erahm]]

Comment on attachment 8987320 [details]
Bug 1470365: Part 1 - Add a compact, read-only, shared-memory string map class.

https://reviewboard.mozilla.org/r/252568/#review260724

Looks good, just a few minor things.

::: dom/ipc/SharedStringMap.h:126
(Diff revisions 1 - 2)
> +private:
> +  /**
> +   * Searches for an entry for the given key. If found, returns true, and
> +   * places its index in the entry array in aIndex.
> +   */
> +  bool Find(const nsCString& aKey, size_t* aIndex);

nit: can you make this const?

::: dom/ipc/SharedStringMap.cpp:131
(Diff revisions 1 - 2)
>  
>  
>    MemMapSnapshot mem;
>    MOZ_TRY(mem.Init(offset));
>  
> -  auto layoutPtr = mem.Get<Layout>();
> +  auto layoutPtr = mem.Get<Header>();

nit: layout => header

::: mfbt/RangedPtr.h:128
(Diff revision 2)
>      MOZ_ASSERT(mRangeEnd == aOther.mRangeEnd);
>    }
>  
> +  template <typename U>
> +  RangedPtr<U>
> +  ReinterpretCast() const

This seems okay, but needs mfbt peer review. Maybe froydnj?

Comment 27

[Kris Maglione [:kmag]]

Comment on attachment 8987320 [details]
Bug 1470365: Part 1 - Add a compact, read-only, shared-memory string map class.

r?froydnj for the RangedPtr change

Comment 28

[Nathan Froyd [:froydnj]]

Comment on attachment 8987320 [details]
Bug 1470365: Part 1 - Add a compact, read-only, shared-memory string map class.

https://reviewboard.mozilla.org/r/252568/#review260768

Kind of gross, but sure.

Comment 29

[Kris Maglione [:kmag]]

Comment on attachment 8987322 [details]
Bug 1470365: Part 3 - Use shared memory for StringBundles loaded in the content process.

https://reviewboard.mozilla.org/r/252572/#review260564

> (not native English speaker but should it be "all the children"?)

Either one works. I think "all children" is probably more standard.

Comment 30

[Kris Maglione [:kmag]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #17)
> Comment on attachment 8987321 [details]
> Bug 1470365: Part 2 - Add StringBundle implementation using shared memory
> maps.
> 
> https://reviewboard.mozilla.org/r/252570/#review260546
> 
> ::: intl/strres/nsStringBundle.cpp:56
> (Diff revision 1)
> > -NS_IMPL_ISUPPORTS(nsStringBundle, nsIStringBundle)
> > +/**
> > + * A set of string bundle URLs which are loaded by content processes, and
> > + * should be allocated in a shared memory region, and then sent to content
> > + * processes.
> > + */
> > +static char kContentBundles[][52] = {
> 
> Can we lock down that you can't add a content process .properties file
> without adding it to this list?
> If that's not possible because of the short-lived bundles, can we alter the
> API to require to pass a flag to retrieve a bundle in the content process
> that is not shared? I'd like to start making the scenario of fetching a
> short-lived bundle in content process greppable.
> 
> ::: intl/strres/nsStringBundle.cpp:960
> (Diff revision 1)
> >    while (!mBundleCache.isEmpty()) {
> >      delete mBundleCache.popFirst();
> >    }
> > +
> > +  // We never flush shared bundles, since their memory cannot be freed, so add
> > +  // them back to the map.
> 
> What happens when you change language at runtime here? We flush string
> bundles and expect to get them refetched/parsed on the next call.

These string bundles are cached in nsContentUtils and never flushed, so that actually doesn't

Comment 31

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b982fcdb2ded44272f2c08be9579b59fd2406364
Bug 1470365: Part 1 - Add a compact, read-only, shared-memory string map class. r=erahm

https://hg.mozilla.org/integration/mozilla-inbound/rev/86fad941c4d4950935568607e66809bbed364b3d
Bug 1470365: Part 2 - Add StringBundle implementation using shared memory maps. r=erahm,gandalf

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc4d3499cbad3391d6831c48fe3147f39c9f5f3e
Bug 1470365: Part 3 - Use shared memory for StringBundles loaded in the content process. r=erahm,smaug

Comment 32

[Kris Maglione [:kmag]]

Comment on attachment 8987320 [details]
Bug 1470365: Part 1 - Add a compact, read-only, shared-memory string map class.

https://reviewboard.mozilla.org/r/252568/#review260446

> Maybe assert that keys, vals, and entries are all the same length.

Forgot to reply to this:

I added an assert that keys and entries are the same length. The values are likely to be shorter, since they're de-duplicated, and lots of string bundles will have multiple keys with the same value.

Comment 33

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/b982fcdb2ded
https://hg.mozilla.org/mozilla-central/rev/86fad941c4d4
https://hg.mozilla.org/mozilla-central/rev/cc4d3499cbad