Use shared memory for preferences in content processes

RESOLVED FIXED in Firefox 63

Status

()

enhancement
RESOLVED FIXED
11 months ago
7 months ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

(Depends on 1 bug, Blocks 2 bugs)

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

(Whiteboard: [overhead:400K][qf:p1:f64][cpstartup:14ms])

Attachments

(12 attachments)

59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
erahm
: review+
njn
: review+
Details
59 bytes, text/x-review-board-request
jld
: review+
njn
: review+
Details
59 bytes, text/x-review-board-request
jld
: review+
Details
59 bytes, text/x-review-board-request
jld
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
Assignee

Description

11 months ago
We currently use about 500K for the preference service in each content process. There are a bunch of ways we can reduce that, including reducing the set of preferences we send to the child process and defining more preferences statically. But the biggest win we can get in the short term is probably using shared memory for the preferences we send to the content process.

The simplest change we could make would probably be sending a static set of preference name strings that exist at the time that we launch the first content process, and pulling preference name strings from that whenever we create a new preference. According to about:memory, that would save us about 115K per process.

The next more extreme step would probably be to snapshot the entire set of preference values the first time we start up a content process, and only store changed values in a hash table. This would similar to what I did for string bundles in bug 1470365.

I'd advocate for the second approach, since it would give us much bigger wins, probably closer to 400K. It might also allow us to do some optimizations, and convert char prefs to UTF-16 internally, so we could return them to JS as external literal strings to avoid copies, since the extra overhead would be shared between processes anyway.


If we used the same approach here that I used for string bundles, that would have the slight disadvantage of requiring O(log n) binary search look-ups for static prefs, rather than O(1) hash table look-ups, but I think that should be reasonable enough. If we're looking up any preference values often enough for that to be an issue, we should probably use caches anyway.

Otherwise, we could of course construct a static hash table rather than a simple sorted list, but I'd like to avoid that complexity if possible.
Assignee

Comment 1

11 months ago
Nick, do you have any opinions on this?
Flags: needinfo?(n.nethercote)
(In reply to Kris Maglione [:kmag] from comment #0)
> We currently use about 500K for the preference service in each content
> process. There are a bunch of ways we can reduce that, including reducing
> the set of preferences we send to the child process and defining more
> preferences statically. But the biggest win we can get in the short term is
> probably using shared memory for the preferences we send to the content
> process.

I agree that shared memory is a good approach here. Assuming it can be made to work, it'll fix the problem for good, and without the need for any kind of one-pref-at-a-time changes, which are agonizing because we have thousands of them.
 
> The simplest change we could make would probably be sending a static set of
> preference name strings that exist at the time that we launch the first
> content process, and pulling preference name strings from that whenever we
> create a new preference. According to about:memory, that would save us about
> 115K per process.

I don't understand this... where would this set be sent?

> The next more extreme step would probably be to snapshot the entire set of
> preference values the first time we start up a content process, and only
> store changed values in a hash table. This would similar to what I did for
> string bundles in bug 1470365.

Would the changed-pref hash table be duplicated in all the content processes, like the full-pref hash table currently is?

> I'd advocate for the second approach, since it would give us much bigger
> wins, probably closer to 400K.

Yeah, aiming high makes sense.

> It might also allow us to do some
> optimizations, and convert char prefs to UTF-16 internally, so we could
> return them to JS as external literal strings to avoid copies, since the
> extra overhead would be shared between processes anyway.

That makes me nervous. The handling of string prefs has evolved over time and is a bit delicate -- e.g. we have [gs]etCharPref() which work with ACString and [gs]etStringPref() which work with AUTF8String and they shouldn't be mixed for individual prefs. I would strongly recommend not making any such changes in a first implementation.

> If we used the same approach here that I used for string bundles, that would
> have the slight disadvantage of requiring O(log n) binary search look-ups
> for static prefs, rather than O(1) hash table look-ups, but I think that
> should be reasonable enough. If we're looking up any preference values often
> enough for that to be an issue, we should probably use caches anyway.

I guess whether this would give acceptable performance is one of the major open questions of this approach. The devil will be in the details, but at a high level it seems to me like a promising idea.
Flags: needinfo?(n.nethercote)
Assignee

Comment 3

11 months ago
(In reply to Nicholas Nethercote [:njn] from comment #2)
> (In reply to Kris Maglione [:kmag] from comment #0)
> > The simplest change we could make would probably be sending a static set of
> > preference name strings that exist at the time that we launch the first
> > content process, and pulling preference name strings from that whenever we
> > create a new preference. According to about:memory, that would save us about
> > 115K per process.
> 
> I don't understand this... where would this set be sent?

Probably as a shared memory file descriptor when we send the shared memory descriptor for the startup prefs.

> > The next more extreme step would probably be to snapshot the entire set of
> > preference values the first time we start up a content process, and only
> > store changed values in a hash table. This would similar to what I did for
> > string bundles in bug 1470365.
> 
> Would the changed-pref hash table be duplicated in all the content
> processes, like the full-pref hash table currently is?

That's a good question. I was initially assuming it would, but it would probably be just as easy to snapshot it in the parent and broadcast the snapshot whenever we send pref changes, as long as it doesn't get too big.

Unfortunately, the bigger the set of runtime changes gets, the more useful a snapshot gets, but also the more impractical it gets.

> > It might also allow us to do some
> > optimizations, and convert char prefs to UTF-16 internally, so we could
> > return them to JS as external literal strings to avoid copies, since the
> > extra overhead would be shared between processes anyway.
> 
> That makes me nervous. The handling of string prefs has evolved over time
> and is a bit delicate -- e.g. we have [gs]etCharPref() which work with
> ACString and [gs]etStringPref() which work with AUTF8String and they
> shouldn't be mixed for individual prefs. I would strongly recommend not
> making any such changes in a first implementation.

Agreed. I was mainly just keeping it in mind as a possible future optimization.

> > If we used the same approach here that I used for string bundles, that would
> > have the slight disadvantage of requiring O(log n) binary search look-ups
> > for static prefs, rather than O(1) hash table look-ups, but I think that
> > should be reasonable enough. If we're looking up any preference values often
> > enough for that to be an issue, we should probably use caches anyway.
> 
> I guess whether this would give acceptable performance is one of the major
> open questions of this approach. The devil will be in the details, but at a
> high level it seems to me like a promising idea.

OK, I'll try to put together a prototype and see what the performance looks like.
Assignee: nobody → kmaglione+bmo
Assignee

Comment 4

11 months ago
This should also help with content process startup perf, since we won't have to load and parse all of our preference files for each content processes.
Whiteboard: [overhead:400K] → [overhead:400K][qf]
Assignee

Updated

11 months ago
Whiteboard: [overhead:400K][qf] → [overhead:400K][qf][cpstartup:14ms]

Updated

11 months ago
Whiteboard: [overhead:400K][qf][cpstartup:14ms] → [overhead:400K][qf:p1:f64][cpstartup:14ms]
Assignee

Comment 5

11 months ago
Current prototype saves about 250K in the parent process and 400K in each child:

├──────403,104 B (00.21%) -- preferences
│      ├──164,136 B (00.09%) ── shared-memory-map
│      ├──115,360 B (00.06%) -- callbacks
│      │  ├──114,912 B (00.06%) ── objects
│      │  └──────448 B (00.00%) ── domains
│      ├───91,488 B (00.05%) ── root-branches
│      ├───22,232 B (00.01%) ── cache-data
│      ├────4,096 B (00.00%) ── pref-name-arena
│      ├────2,112 B (00.00%) ── pref-values
│      ├────2,080 B (00.00%) ── hash-table
│      ├────1,472 B (00.00%) ── string-values
│      └──────128 B (00.00%) ── misc


├──────92,280 B (00.43%) -- preferences
│      ├──48,672 B (00.23%) -- callbacks
│      │  ├──48,672 B (00.23%) ── objects
│      │  └───────0 B (00.00%) ── domains
│      ├──19,992 B (00.09%) ── cache-data
│      ├──13,472 B (00.06%) ── root-branches
│      ├───4,096 B (00.02%) ── pref-name-arena
│      ├───2,368 B (00.01%) ── pref-values
│      ├───2,080 B (00.01%) ── hash-table
│      ├───1,472 B (00.01%) ── string-values
│      └─────128 B (00.00%) ── misc


├──────92,280 B (00.43%) -- preferences
│      ├──48,672 B (00.22%) -- callbacks
│      │  ├──48,672 B (00.22%) ── objects
│      │  └───────0 B (00.00%) ── domains
│      ├──19,992 B (00.09%) ── cache-data
│      ├──13,472 B (00.06%) ── root-branches
│      ├───4,096 B (00.02%) ── pref-name-arena
│      ├───2,368 B (00.01%) ── pref-values
│      ├───2,080 B (00.01%) ── hash-table
│      ├───1,472 B (00.01%) ── string-values
│      └─────128 B (00.00%) ── misc


├──────84,328 B (00.50%) -- preferences
│      ├──42,576 B (00.25%) -- callbacks
│      │  ├──42,576 B (00.25%) ── objects
│      │  └───────0 B (00.00%) ── domains
│      ├──18,456 B (00.11%) ── cache-data
│      ├──13,152 B (00.08%) ── root-branches
│      ├───4,096 B (00.02%) ── pref-name-arena
│      ├───2,368 B (00.01%) ── pref-values
│      ├───2,080 B (00.01%) ── hash-table
│      ├───1,472 B (00.01%) ── string-values
│      └─────128 B (00.00%) ── misc
Assignee

Comment 6

11 months ago
As for performance, it actually looks like the binary search of the shared memory DB is slightly faster than the PLDHashTable lookup, even when the hashtable is mostly empty and only contains overrides.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 19

11 months ago
mozreview-review
Comment on attachment 8989850 [details]
Bug 1471025: Part 1 - Store preference access counts in a separate hashtable.

https://reviewboard.mozilla.org/r/254814/#review261740

::: modules/libpref/Preferences.cpp:1083
(Diff revision 1)
> +static AccessCountsHashTable* gAccessCounts;
> +
> +static void
> +AddAccessCount(const nsACString& aPrefName)
> +{
> +  auto& count = gAccessCounts->GetOrInsert(aPrefName);

I would prefer `uint32_t&`. It's clearer and barely any longer and more consistent with the Moz style guide.
Attachment #8989850 - Flags: review?(n.nethercote) → review+

Comment 20

11 months ago
mozreview-review
Comment on attachment 8989851 [details]
Bug 1471025: Part 2 - Add a helper class creating and accessing shared preference map snapshots.

https://reviewboard.mozilla.org/r/254816/#review261742

::: modules/libpref/Preferences.cpp:320
(Diff revision 1)
>      }
>    }
>  };
>  
> +template<>
> +bool

This makes me nervous, that bool and uint32_t could be mixed up somehow.

::: modules/libpref/Preferences.cpp:508
(Diff revision 1)
> +      AddToMap<bool>(aMap);
> +    } else if (IsTypeInt()) {
> +      AddToMap<int32_t>(aMap);
> +    } else if (IsTypeString()) {
> +      AddToMap<nsDependentCString>(aMap);
> +    }

Probably worth MOZ_CRASHing if we don't match any conditions here.

::: modules/libpref/Preferences.cpp:3128
(Diff revision 1)
> +                         UNITS_BYTES,
> +                         gSharedMap->MapSize(),
> +                         "The shared memory mapping used to share a "
> +                         "snapshot of preference values across processes.");
> +    } else {
> +      MOZ_COLLECT_REPORT("preferences/shared-memory-map",

This doesn't seem useful. The value for all the child processes will be the same as the value for the parent process. Just omit it? (And only measure above if it's the parent process.)

::: modules/libpref/Preferences.cpp:3561
(Diff revision 1)
>    gContentProcessPrefsAreInited = true;
>  #endif
>  }
>  
> +/* static */ FileDescriptor
> +Preferences::GetSnapshot(size_t* aSize)

`GetSnapshot` doesn't make me think of this kind of initializer. Something more like `EnsureSnapshot` does.

::: modules/libpref/SharedPrefMap.h:21
(Diff revision 1)
> +
> +namespace mozilla {
> +
> +class SharedPrefMapBuilder;
> +
> +/**

The rest of libpref doesn't use any /* */ or /** */ comments; it only uses //. For consistency, please do likewise, here and in the other patches.

::: modules/libpref/SharedPrefMap.h:26
(Diff revision 1)
> +/**
> + * This class provides access to a compact, read-only copy of a preference
> + * database, backed by a shared memory buffer which can be shared between
> + * processes. All state data for the database is stored in the shared memory
> + * region, so individual instances require no dynamic memory allocation.
> + * Further, all strings returned from this API are literal strings with pointers

Do you mean "literal strings" or do you mean something more like "immutable C strings"?

::: modules/libpref/SharedPrefMap.h:27
(Diff revision 1)
> + * This class provides access to a compact, read-only copy of a preference
> + * database, backed by a shared memory buffer which can be shared between
> + * processes. All state data for the database is stored in the shared memory
> + * region, so individual instances require no dynamic memory allocation.
> + * Further, all strings returned from this API are literal strings with pointers
> + * into the shared memory region, which means that they can be moved around

It's unclear what "moved around" means here. Can you reword?

::: modules/libpref/SharedPrefMap.h:32
(Diff revision 1)
> + * into the shared memory region, which means that they can be moved around
> + * without triggering further allocations.
> + *
> + * The set of entries is stored in sorted order by preference name, so look-ups
> + * are done by binary search. This means that look-ups have O(log n) complexity,
> + * rather than the O(n) complexity of a dynamic hashtable. Consumers should keep

O(1)?

::: modules/libpref/SharedPrefMap.h:73
(Diff revision 1)
> +   *   entries in the header, described below.
> +   *
> +   * Each entry stores its name string and values as indices into these blocks,
> +   * as documented in the Entry struct, but with some important optimizations:
> +   *
> +   * - Boolean values are always stored inline. Both the default and user

I haven't got far through the patch yet, but I wonder if storing bools out-of-line would simplify things -- one less special case to worry about.

::: modules/libpref/SharedPrefMap.h:98
(Diff revision 1)
> +   *
> +   * - For preferences with no user value, the entries in the default value are
> +   *   de-duplicated. All preferences with the same default value (and no user
> +   *   value) point to the same index in the default value array.
> +   */
> +  struct Header

This format is flexible enough that imagining it is a challenge. What would help immensely would be a small example involving a handful of prefs. Would you mind adding one?

::: modules/libpref/SharedPrefMap.h:218
(Diff revision 1)
> +    bool HasDefaultValue() const { return mEntry->mHasDefaultValue; }
> +    bool HasUserValue() const { return mEntry->mHasUserValue; }
> +    bool IsLocked() const { return mEntry->mIsLocked; }
> +    bool IsSticky() const { return mEntry->mIsSticky; }
> +
> +    bool GetBoolValue(bool aGetDefault = false) const

libpref uses `PrefValueKind` consistently to distinguish default and user values, rather than bool. Please do likewise in this class.

::: modules/libpref/SharedPrefMap.h:260
(Diff revision 1)
> +
> +    // Returns the entry's index in the map, as understood by GetKeyAt() and
> +    // GetValueAt().
> +    size_t Index() const { return mEntry - mMap->Entries().get(); }
> +
> +    }

`GetStringValue()` is missing a closing brace, and then there is a stray closing brace here, which means that `GetBareStringValue()` and `Index()` are defined within `GetStringValue()`. Does this compile?

::: modules/libpref/SharedPrefMap.h:278
(Diff revision 1)
> +  private:
> +    const SharedPrefMap* const mMap;
> +    const Entry* mEntry;
> +  };
> +
> +  // Note: These constructors are infallible, since the preference database is

A pet peeve of mine: s/since/because/

::: modules/libpref/SharedPrefMap.h:280
(Diff revision 1)
> +    const Entry* mEntry;
> +  };
> +
> +  // Note: These constructors are infallible, since the preference database is
> +  // critical to platform functionality, and we cannot operate without it.
> +  explicit SharedPrefMap(const FileDescriptor&, size_t);

This ctor has two arguments. Does it need to be `explicit`?

::: modules/libpref/SharedPrefMap.h:314
(Diff revision 1)
> +   */
> +  uint32_t Count() const { return EntryCount(); }
> +
> +  /**
> +   * Returns the string entry at the given index. Keys are guaranteed to be
> +   * sorted lexographically.

Typo: lexicographically

::: modules/libpref/SharedPrefMap.h:357
(Diff revision 1)
> +protected:
> +  ~SharedPrefMap() = default;
> +
> +private:
> +  template<typename T>
> +  using StringTable = SharedStringMap::StringTable<T>;

It's a bit gross that libpref has to reach into SharedStringMap. It would be nicer if StringTable was a standalone building block.

::: modules/libpref/SharedPrefMap.h:407
(Diff revision 1)
> +  {
> +    auto& block = GetHeader().mValueStrings;
> +    return { { &mMap.get<uint8_t>()[block.mOffset], block.mSize } };
> +  }
> +
> +  loader::AutoMemMap mMap;

This is `AutoMemMap` from js/xpconnect/loader/AutoMemMap.h, right? Ugh, that should really be in MFBT.

::: modules/libpref/SharedPrefMap.h:461
(Diff revision 1)
> +  using SharedStringMap = mozilla::dom::ipc::SharedStringMap;
> +  using SharedStringMapBuilder = mozilla::dom::ipc::SharedStringMapBuilder;
> +
> +  // An opaque descriptor the index of a preference entry in a value array,
> +  // which can be converted numeric index after the ValueTableBuilder is
> +  // finalized.

This sentence is missing a word. Should be "descriptor of the index", perhaps?

::: modules/libpref/SharedPrefMap.h:495
(Diff revision 1)
> +   *
> +   * To deal with this, when entries are added, we return an opaque ValueIndex
> +   * struct, from which we can calculate the final index after the map has been
> +   * finalized.
> +   */
> +  template<typename HashKey, typename ValueType_>

Why the trailing '_'?

::: modules/libpref/SharedPrefMap.h:542
(Diff revision 1)
> +    uint16_t GetIndex(const ValueIdx& aIndex) const
> +    {
> +      if (aIndex.mHasUserValue) {
> +        return aIndex.mIndex;
> +      }
> +      return aIndex.mIndex + UserCount();

I'd do this:
```
return aIndex.mHasUserValue
     ? aIndex.mIndex
     : aIndex.mIndex + UserCount();
```
because each alternative has equal "weight". (You could even factor out the aIndex.mIndex part.)

::: modules/libpref/SharedPrefMap.h:716
(Diff revision 1)
> +
> +    uint8_t mType : 2;
> +    uint8_t mHasDefaultValue : 1;
> +    uint8_t mHasUserValue : 1;
> +    uint8_t mIsSticky : 1;
> +    uint8_t mIsLocked : 1;

It's unfortunate that `Pref`, `SharedPrefMap::Entry`, and `SharedPrefMapBuilder::Entry` are so similar. Is there some way to avoid the repetition?

::: modules/libpref/SharedPrefMap.h:739
(Diff revision 1)
> +        MOZ_ASSERT_UNREACHABLE("Invalid pref type");
> +        return { false, false };
> +    }
> +  }
> +
> +  UniqueStringTableBuilder<char> mKeyTable{ 4000 };

A comment about why 4000 is a good number would be useful.

::: modules/libpref/SharedPrefMap.h:747
(Diff revision 1)
> +
> +  ValueTableBuilder<nsUint32HashKey, uint32_t> mIntValueTable;
> +  ValueTableBuilder<nsGenericHashKey<StringEntry>, StringEntry>
> +    mStringValueTable;
> +
> +  nsTArray<Entry> mEntries{ 4000 };

Likewise here. Giving this value a name would probably help, too.

::: modules/libpref/SharedPrefMap.cpp:227
(Diff revision 1)
> +
> +  auto* entryPtr = reinterpret_cast<SharedPrefMap::Entry*>(&headerPtr[1]);
> +  for (auto* entry : entries) {
> +    *entryPtr++ = {
> +      entry->mKey,          GetValue(*entry),
> +      entry->mType,         entry->mHasDefaultValue,

This looks like it hasn't been clang-formatted.
Attachment #8989851 - Flags: review?(n.nethercote) → review+

Comment 21

11 months ago
mozreview-review
Comment on attachment 8989852 [details]
Bug 1471025: Part 3a - Pass shared preference map to (non-Android) content processes at startup.

https://reviewboard.mozilla.org/r/254818/#review261752

Seems ok for what it is, but the obvious question: can these two FDs be combined? They are very closely related.

::: dom/ipc/ContentProcess.cpp:193
(Diff revision 1)
> +      }
> +      // ContentParent uses %zu to print a word-sized unsigned integer. So
> +      // even though strtoull() returns a long long int, it will fit in a
> +      // uintptr_t.
> +      char* str = aArgv[i];
> +      prefMapSize = Some(strtoull(str, &str, 10));

Factor out this repetition of `strtoull`, like you did the similar cases above?
Attachment #8989852 - Flags: review?(n.nethercote) → review+

Comment 22

11 months ago
mozreview-review
Comment on attachment 8989855 [details]
Bug 1471025: Part 4 - Add a wrapper class that can access either static or dynamic prefs.

https://reviewboard.mozilla.org/r/254824/#review261756

::: commit-message-11c6f:13
(Diff revision 1)
> +think the performance characteristics would be acceptable for preferences
> +code. And since the wrapper classes used for static prefs are temporary, they
> +would add the additional snag of figuring out how to keep a valid pointer
> +alive.
> +
> +So, instead, this patch adds a wrapper class that can access eaither type of

Type: eaither

::: modules/libpref/Preferences.cpp:551
(Diff revision 1)
>      }
>  
>      return strcmp(mName, aPrefName) == 0;
>    }
>  
> -  nsresult GetBoolValue(PrefValueKind aKind, bool* aResult)
> +  bool GetBoolValue(bool aGetDefault = false) const

Again, please use `PrefValueKind` instead of `bool` to indicate the user/default distinction. Likewise for the methods below.

::: modules/libpref/Preferences.cpp:557
(Diff revision 1)
> -    if (!IsTypeBool()) {
> -      return NS_ERROR_UNEXPECTED;
> +    MOZ_ASSERT(IsTypeBool());
> +    MOZ_ASSERT(aGetDefault ? HasDefaultValue() : HasUserValue());
> -    }
>  
> -    if (aKind == PrefValueKind::Default || IsLocked() || !mHasUserValue) {
> -      // Do we have a default?
> +    if (aGetDefault) {
> +      return mDefaultValue.mBoolVal;

The `?:` operator is a good choice here. Likewise for the methods below.

::: modules/libpref/Preferences.cpp:1064
(Diff revision 1)
> +  FORWARD(nsCString, NameString)
> +  FORWARD(PrefType, Type)
> +#undef FORWARD
> +
> +#define FORWARD(retType, method)                                               \
> +  retType method(bool aGetDefault = false) const                               \

Ditto for the use of `bool`.

::: modules/libpref/Preferences.cpp:4759
(Diff revision 1)
>  /* static */ bool
>  Preferences::IsLocked(const char* aPrefName)
>  {
>    NS_ENSURE_TRUE(InitStaticMembers(), false);
>  
> -  Pref* pref = pref_HashTableLookup(aPrefName);
> +  auto pref = pref_Lookup(aPrefName);

Use `Maybe<PrefWrapper>` here instead of `auto` for consistency with the above code.
Attachment #8989855 - Flags: review?(n.nethercote) → review+

Comment 23

11 months ago
mozreview-review
Comment on attachment 8989856 [details]
Bug 1471025: Part 5 - Add a range iterator helper for iterating both static and dynamic preferences.

https://reviewboard.mozilla.org/r/254826/#review261760

::: modules/libpref/Preferences.cpp:1265
(Diff revision 1)
> +    friend class PrefsHashIter;
> +
> +    PrefsHashIter& mParent;
> +    bool mDone;
> +
> +    Elem(PrefsHashIter& iter, bool done)

Nit: `aIter`, `aDone`

::: modules/libpref/Preferences.cpp:1410
(Diff revision 1)
> +    friend class PrefsIter;
> +
> +    PrefsIter& mParent;
> +    bool mDone;
> +
> +    Elem(PrefsIter& iter, bool done)

Nit: `aIter`, `aDone`.

::: modules/libpref/SharedPrefMap.h:250
(Diff revision 1)
>        auto& stringEntry =
>          (aGetDefault ? mMap->DefaultStringValues()[mEntry->mValue.mIndex]
>                       : mMap->UserStringValues()[mEntry->mValue.mIndex]);
>  
>        return mMap->ValueTable().Get(stringEntry);
> +    }

Oh, this fixes the problem with the stray closing brace that I identified in patch 2.
Attachment #8989856 - Flags: review?(n.nethercote) → review+

Comment 24

11 months ago
mozreview-review
Comment on attachment 8989857 [details]
Bug 1471025: Part 6 - Optimize preference lookups while dispatching callbacks.

https://reviewboard.mozilla.org/r/254828/#review261764

This is a bit of a hack. Do you have measurements indicating that it makes a noticeable difference?

::: commit-message-7fd26:4
(Diff revision 1)
> +Bug 1471025: Part 6 - Optimize preference lookups while dispatching callbacks. r?njn
> +
> +Since lookups in the snapshotted hashtable are currently O(log n) rather than
> +O(n), they're expected to be slightly more expensive than the previous

O(1)?

::: modules/libpref/Preferences.cpp:1542
(Diff revision 1)
>    return entry->mPref;
>  }
>  
> +// While notifying preference callbacks, this holds the wrapper for the
> +// preference being notified, in order to optimize lookups.
> +static const PrefWrapper* gCallbackPref;

Maybe add a note that this is safe because all lookups only happen on the main thread.

::: modules/libpref/Preferences.cpp:1547
(Diff revision 1)
> +static const PrefWrapper* gCallbackPref;
> +
>  Maybe<PrefWrapper>
>  pref_Lookup(const char* aPrefName)
>  {
>    Maybe<PrefWrapper> result;

A main-thread-only assertion might be good here.
Assignee

Comment 25

11 months ago
mozreview-review-reply
Comment on attachment 8989851 [details]
Bug 1471025: Part 2 - Add a helper class creating and accessing shared preference map snapshots.

https://reviewboard.mozilla.org/r/254816/#review261742

> This makes me nervous, that bool and uint32_t could be mixed up somehow.

I'm not sure how they could. The types need to be explicitly specified, and `Get<uint32_t>` just results in a linking error since it's not defined.

> Do you mean "literal strings" or do you mean something more like "immutable C strings"?

Yes, but I'm using the term "literal strings" in the sense of nsLiteralString, i.e., nsCStrings with the LITERAL flag.

> O(1)?

Er, yes.

> I haven't got far through the patch yet, but I wonder if storing bools out-of-line would simplify things -- one less special case to worry about.

It might, but if it did, it wouldn't be by very much, and it would require significantly more memory.

> `GetStringValue()` is missing a closing brace, and then there is a stray closing brace here, which means that `GetBareStringValue()` and `Index()` are defined within `GetStringValue()`. Does this compile?

Yeah, sorry, rebase botch. Fixed in a later patch.

> This ctor has two arguments. Does it need to be `explicit`?

Nope. This is inherited from SharedStringMap, where the constructor originally had one argument but needed a second one added later.

> This is `AutoMemMap` from js/xpconnect/loader/AutoMemMap.h, right? Ugh, that should really be in MFBT.

Yeah. I'll move it out of loader eventually, but it probably needs to be in xpcom or ipc rather than MFBT.

> Why the trailing '_'?

Because I initially needed this to be available outside of the class definition, and I can't do `using ValueType = ValueType`

> It's unfortunate that `Pref`, `SharedPrefMap::Entry`, and `SharedPrefMapBuilder::Entry` are so similar. Is there some way to avoid the repetition?

I thought about it. I'm not sure there's a good way that wouldn't make things more confusing.

> A comment about why 4000 is a good number would be useful.

Yeah, I meant to make this a constant before I submitted, but forgot.

> This looks like it hasn't been clang-formatted.

Heh. It has. I never would have formatted it like this, but clang-format decided to.
Assignee

Comment 26

11 months ago
mozreview-review-reply
Comment on attachment 8989852 [details]
Bug 1471025: Part 3a - Pass shared preference map to (non-Android) content processes at startup.

https://reviewboard.mozilla.org/r/254818/#review261752

> Seems ok for what it is, but the obvious question: can these two FDs be combined? They are very closely related.

Unfortunately, no. The shared map FD needs to be read-only, so that can be shared between processes, but the data in the serialized preferences FD will need to change for every process we launch.
Assignee

Comment 27

11 months ago
(In reply to Nicholas Nethercote [:njn] from comment #24)
> Comment on attachment 8989857 [details]
> Bug 1471025: Part 6 - Optimize preference lookups while dispatching
> callbacks.
> 
> https://reviewboard.mozilla.org/r/254828/#review261764
> 
> This is a bit of a hack. Do you have measurements indicating that it makes a
> noticeable difference?

Not as such. I have measurements that pref lookups are expensive enough to show up in startup profiles (even without the shared map), that this is much cheaper, and that the hit rate for observers is pretty high. I don't have before and after numbers that say how much difference it makes, though.

In practice, it depends a lot on how many static varcache preferences have user values, and how many non-static pref caches are either defined before preferences are loaded or have user values. I can check how many that winds up being in a clean profile, or in one of my daily use profiles, but I don't know how those numbers relate to the real world...
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
> It might, but if it did, it wouldn't be by very much, and it would require significantly more memory.

I thought that because of de-duplication that there would be one "true" entry and one "false" entry shared by all the bool prefs? I might be overlooking something.

Comment 52

11 months ago
mozreview-review
Comment on attachment 8989851 [details]
Bug 1471025: Part 2 - Add a helper class creating and accessing shared preference map snapshots.

https://reviewboard.mozilla.org/r/254816/#review261970

::: modules/libpref/SharedPrefMap.h:27
(Diff revisions 1 - 3)
> +// This number is used to determine initial allocation sizes for data structures
> +// when building the shared preference map. As a rule, prefer to over-shoot
> +// rather than under-shoot to avoid unnecessary reallocations/rehashes, but
> +// prefer a number one-less-than a power of two rather than an exact power of
> +// two to avoid doubling initial hashtable sizes.
> +constexpr size_t kExpectedPrefCount = 4000;

One less than a power of two would be 4095. Having said that, I don't understand why you can't use a power of two. Is it because this also involves an nsTArray? (In which case you really want a bit less than a power of two, to leave space for the header, exactly as 4000 does.)

::: modules/libpref/SharedPrefMap.h:39
(Diff revisions 1 - 3)
> - * processes. All state data for the database is stored in the shared memory
> - * region, so individual instances require no dynamic memory allocation.
> - * Further, all strings returned from this API are literal strings with pointers
> - * into the shared memory region, which means that they can be moved around
> - * without triggering further allocations.
> - *
> +// region, so individual instances require no dynamic memory allocation.
> +//
> +// Further, all strings returned from this API are nsLiteralCStrings with
> +// pointers into the shared memory region, which means that they can be copied
> +// into new nsCString instances without additional allocations. For instance,
> +// the following (where `pref` is  Pref object) will not cause any string

Nit: two spaces before `Pref`.

::: modules/libpref/SharedPrefMap.h:45
(Diff revisions 1 - 3)
> - * The set of entries is stored in sorted order by preference name, so look-ups
> - * are done by binary search. This means that look-ups have O(log n) complexity,
> - * rather than the O(n) complexity of a dynamic hashtable. Consumers should keep
> - * this in mind when planning their accesses.
> - *
> - * Important: The mapped memory created by this class is persistent. Once an
> +// copies, memory allocations, or atomic refcount changes:
> +//
> +//   nsCString prefName(pref.NameString());
> +//
> +// whereas if we returned a nsDependentCString or a dynamically allocated
> +// nsCString, it would.

Why would an nsDependentCString cause an allocation?

::: modules/libpref/SharedPrefMap.h:116
(Diff revisions 1 - 3)
> -   */
> +  //
> +  // For example, a preference database containing:
> +  //
> +  // +---------+-------------------------------+-------------------------------+
> +  // | Name    | Default Value | User Value    |                               |
> +  // +---------+---------------+---------------+-------------------------------+

Thanks for adding this example, it's very helpful. My only suggestion would be to use '|' instead of '/' for the vertical lines :)

::: dom/ipc/StringTable.h:36
(Diff revision 3)
> +  {
> +    return mOffset == aOther.mOffset;
> +  }
> +};
> +
> +template <typename StringType>

This file needs an overview comment, either at the top, or above this class.

Comment 53

11 months ago
mozreview-review
Comment on attachment 8989852 [details]
Bug 1471025: Part 3a - Pass shared preference map to (non-Android) content processes at startup.

https://reviewboard.mozilla.org/r/254818/#review261978

::: modules/libpref/Preferences.h:66
(Diff revision 3)
>  #endif
>  
>  #ifdef XP_UNIX
>  // XXX: bug 1440207 is about improving how fixed fds such as this are used.
>  static const int kPrefsFileDescriptor = 8;
> +static const int kPrefMapFileDescriptor = 9;

It would be useful to add a brief comment explaining why these FDs can't be combined.
Assignee

Comment 54

11 months ago
(In reply to Nicholas Nethercote [:njn] from comment #51)
> > It might, but if it did, it wouldn't be by very much, and it would require significantly more memory.
> 
> I thought that because of de-duplication that there would be one "true"
> entry and one "false" entry shared by all the bool prefs? I might be
> overlooking something.

There would be for prefs with only a default value, but user values aren't de-duplicated in the current scheme, and we tend to have a lot of bool preferences with user values.
Assignee

Comment 55

11 months ago
mozreview-review-reply
Comment on attachment 8989851 [details]
Bug 1471025: Part 2 - Add a helper class creating and accessing shared preference map snapshots.

https://reviewboard.mozilla.org/r/254816/#review261970

> One less than a power of two would be 4095. Having said that, I don't understand why you can't use a power of two. Is it because this also involves an nsTArray? (In which case you really want a bit less than a power of two, to leave space for the header, exactly as 4000 does.)

I mean, people have a tendency to use power-of-two sizes, which doesn't work well in this case, so if you're close to one, you should aim for under. The nsTArray case is fine, but when you pass 4096 as the initial size to a PLDHashTable, it allocates space for 8192 entries. When you pass 4095, it allocates space for 4096.

> Why would an nsDependentCString cause an allocation?

When you assign a value to a nsCString instance, it always causes a copy unless the value is refcounted or literal. That's the case for nsDependentCString mostly because it has no lifetime guarantees, so you're not supposed to use its value beyond the lifetime of its (presumably stack-allocated) instance.

> Thanks for adding this example, it's very helpful. My only suggestion would be to use '|' instead of '/' for the vertical lines :)

Heh. I did, but reviewboard "helpfully" italicizes comments, which makes it look like a / in some fonts.
> when you pass 4096 as the initial size to a
> PLDHashTable, it allocates space for 8192 entries. When you pass 4095, it
> allocates space for 4096.

That's not right. The constructor's comment says:

"The table's initial capacity is chosen such that |aLength| elements can be inserted without rehashing; if |aLength| is a power-of-two, this capacity will be |2*length|."

The capacity is chosen such that the given length makes the table between 25% and 75% full. So any length in the range 2048--6144 (or possibly 2049--6143... I'm not certain about the edge cases) will result in a capacity of 8192.

> Heh. I did, but reviewboard "helpfully" italicizes comments, which makes it
> look like a / in some fonts.

lol

Comment 57

11 months ago
mozreview-review
Comment on attachment 8989858 [details]
Bug 1471025: Part 7a - Look up preferences from both dynamic and shared preference tables.

https://reviewboard.mozilla.org/r/254830/#review262054

This all looks reasonable, but given how simple prefs is, conceptually -- just keys with default and/or user values, not that bad right? -- this stuff is surprisingly complex and subtle with edge cases like locking. It would be easy for me to overlook problems. Hopefully there aren't any :)

::: modules/libpref/Preferences.cpp:1203
(Diff revision 3)
> +{
> +  MOZ_ASSERT(aWrapper.is<SharedPrefMap::Pref>());
> +  auto pref = aWrapper.as<SharedPrefMap::Pref>();
> +
> +  MOZ_ASSERT(IsTypeNone());
> +  MOZ_ASSERT(!strcmp(mName, pref.Name()));

I prefer `strcmp(...) == 0`

::: modules/libpref/Preferences.cpp:1653
(Diff revision 3)
>    } else if (Pref* pref = pref_HashTableLookup(aPrefName)) {
> +    if (aIncludeTypeNone || !pref->IsTypeNone()) {
> -    result.emplace(pref);
> +      result.emplace(pref);
> -  }
> +    }
> +  } else if (gSharedMap) {
> +    auto pref = gSharedMap->Get(aPrefName);

Please use the type instead of `auto` here.

::: modules/libpref/Preferences.cpp:1666
(Diff revision 3)
>  
> +static Result<Pref*, nsresult>
> +pref_LookupForModify(const char* aPrefName,
> +                     const std::function<bool(const PrefWrapper&)>& aCheckFn)
> +{
> +  auto wrapper = pref_Lookup(aPrefName, /* includeTypeNone */ true);

ditto

::: modules/libpref/Preferences.cpp:4161
(Diff revision 3)
>  {
>    ENSURE_PARENT_PROCESS("Preferences::ResetPrefs", "all prefs");
>  
> +  if (gSharedMap) {
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }

Hmm, this is interesting. I wonder if it might cause problems, but I can't think of anything specific.

::: modules/libpref/test/unit_ipc/test_large_pref.js:55
(Diff revision 3)
>    let ps = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefService);
>    let defaultBranch = ps.getDefaultBranch("");
>  
>    let isParent = isParentProcess();
>    if (isParent) {
> +    // Preferences will large values will still appear in the shared memory

s/will/with/?
Attachment #8989858 - Flags: review?(n.nethercote) → review+

Comment 58

11 months ago
mozreview-review
Comment on attachment 8989859 [details]
Bug 1471025: Part 7b - Don't load preference files in the content process.

https://reviewboard.mozilla.org/r/254832/#review262056

::: modules/libpref/Preferences.cpp:759
(Diff revision 3)
>          SetIsLocked(true);
>        }
>        if (!ValueMatches(PrefValueKind::Default, aType, aValue)) {
>          mDefaultValue.Replace(mHasDefaultValue, Type(), aType, aValue);
>          mHasDefaultValue = true;
> -        if (!aFromInit) {
> +        OnChanged();

Is the `aFromInit` argument still used?

::: modules/libpref/Preferences.cpp:1581
(Diff revision 3)
>  {
>    NotifyCallbacks(aPrefName, &aPref);
>  }
>  
> -#define PREF_HASHTABLE_INITIAL_LENGTH 1024
> +constexpr size_t kHashTableInitialLengthParent = 3000;
> +constexpr size_t kHashTableInitialLengthContent = 63;

Brief comments about these number choices would be helpful.

::: modules/libpref/Preferences.cpp:4746
(Diff revision 3)
>  {
>    // Initialize static prefs before prefs from data files so that the latter
>    // will override the former.
>    StaticPrefs::InitAll(aIsStartup);
>  
> +  if (XRE_IsContentProcess()) {

Hmm... everywhere else in libpref we use `!XRE_IsParentProcess()`, which has a slightly different meaning to `XRE_IsContentProcess()`. Unless there is a good reason for it, I'd prefer consistency.
Attachment #8989859 - Flags: review?(n.nethercote) → review+

Comment 59

11 months ago
mozreview-review
Comment on attachment 8989860 [details]
Bug 1471025: Part 7c - Clear the dynamic hashtable in the parent process after snapshotting.

https://reviewboard.mozilla.org/r/254834/#review262058

Oh good; I was just wondering about the duplication of the data in the main process :)

::: modules/libpref/Preferences.cpp
(Diff revision 3)
>      , mType(static_cast<uint32_t>(PrefType::None))
>      , mIsSticky(false)
>      , mIsLocked(false)
>      , mHasDefaultValue(false)
>      , mHasUserValue(false)
> -    , mHasChangedSinceInit(false)

Nice! That change-tracking code was always fiddly, so it's good to see it go away.

::: modules/libpref/Preferences.cpp:4008
(Diff revision 3)
>        pref->AddToMap(builder);
>      }
>  
>      gSharedMap = new SharedPrefMap(std::move(builder));
> +
> +    gHashTable->ClearAndPrepareForLength(kHashTableInitialLengthContent);

Please add a comment here explaining what is happening and why it's ok.
Attachment #8989860 - Flags: review?(n.nethercote) → review+
Assignee

Comment 60

11 months ago
mozreview-review-reply
Comment on attachment 8989858 [details]
Bug 1471025: Part 7a - Look up preferences from both dynamic and shared preference tables.

https://reviewboard.mozilla.org/r/254830/#review262054

> This all looks reasonable, but given how simple prefs is, conceptually -- just keys with default and/or user values, not that bad right? -- this stuff is surprisingly complex and subtle with edge cases like locking.

I think ideally it would be conceptually simple, but locked prefs and default values make it complicated. We can pretty easily ignore all of that complexity when we send changes to the child process, because we already know what the visible effect of a change will be by the time we need to send a change.

But for the parent, we really do need to make sure we update the full state correctly all the time. For instance, if we have something like:

    setUser(pref, true);
    snapshotPrefs();
    setDefault(pref, false);
    lockPref(pref);

The setDefault initially has no visible effect, so we don't send any change to the child. Then we call lockPref, and the default value takes precedence over the user value, and we send both changes to the child.

When that change is made in the parent process, we need to make sure we record the setDefault change, even though it has no visible effect. Which means that we need to copy the value from the snapshot to the hashmap.

I suppose we could ignore a lot of that complexity if we decided not to worry about copying entries to the dynamic hashtable if the changes are no-ops, but that makes me nervous. Dynamic preference values add up pretty quickly, and we definitely make a fair number of no-op setPref calls. And lockPref calls, in particular, are used a lot by enterprises... and it's really hard to know what their actual usage patterns are.


So, basically, I tried to be as comprehensive and correct as possible, and tried to make sure I added tests for most of the edge cases I could think of.

> Hmm, this is interesting. I wonder if it might cause problems, but I can't think of anything specific.

I don't think it will. It's only used in xpcshell tests, none of which launch child processes, so they should never wind up with a snapshot, and should continue to work.

When we're not in a restricted xpcshell test... I think we probably want this to fail, anyway. Calling it during an active browser session would pretty much brick it.

Comment 61

11 months ago
mozreview-review
Comment on attachment 8989861 [details]
Bug 1471025: Part 8 - Add tests for shared memory preferences.

https://reviewboard.mozilla.org/r/254836/#review262066
Attachment #8989861 - Flags: review?(n.nethercote) → review+
Assignee

Comment 62

11 months ago
mozreview-review-reply
Comment on attachment 8989859 [details]
Bug 1471025: Part 7b - Don't load preference files in the content process.

https://reviewboard.mozilla.org/r/254832/#review262056

> Is the `aFromInit` argument still used?

It's used in SetUserValue. I decided to keep it in SetDefaultValue for consistency, but I don't really have any objection to removing it either.
Overall, this is a major change to libpref's functionality and adds a lot of new code, which makes me nervous. Having said that: it's a necessary change; I am pleased it has been done; the overall approach is reasonable; and the code quality looks good. Regressions are possible but that's unavoidable for a change of this size.

One more request: can you write a brief summary (just as a comment in this bug) that covers parent process startup, content process startup and IPC, and what happens on pref value changes? I have a picture in my head that has formed while reading this patch stack, but it was a lot of code, and it would be good to see how that matches the picture in your head. Thank you.

When it lands, I recommend emailing dev-platform and firefox-dev to let people know, so they can keep an eye out for any regressions.
> So, basically, I tried to be as comprehensive and correct as possible, and tried to make sure I added tests for most of the edge cases I could think of.

I approve this message!

Comment 65

11 months ago
mozreview-review
Comment on attachment 8989857 [details]
Bug 1471025: Part 6 - Optimize preference lookups while dispatching callbacks.

https://reviewboard.mozilla.org/r/254828/#review261988

::: commit-message-7fd26:4
(Diff revision 3)
> +Bug 1471025: Part 6 - Optimize preference lookups while dispatching callbacks. r?njn
> +
> +Since lookups in the snapshotted hashtable are currently O(log n) rather than
> +O(n), they're expected to be slightly more expensive than the previous

O(1)
Attachment #8989857 - Flags: review?(n.nethercote) → review+

Comment 66

11 months ago
mozreview-review
Comment on attachment 8989851 [details]
Bug 1471025: Part 2 - Add a helper class creating and accessing shared preference map snapshots.

https://reviewboard.mozilla.org/r/254816/#review262016

Thanks for thoroughly commenting this, it made it much easier to review. Overall I think it's a good implementation, I have some super minor feedback.

::: modules/libpref/SharedPrefMap.h:212
(Diff revision 3)
> +  // +------------------------------------+------------------------------------+
> +  // | User string values                 | Default string values              |
> +  // +-------+----------------------------+-------+----------------------------+
> +  // | Index | Contents[1]                | Index | Contents[1]                |
> +  // +-------+----------------------------+-------+----------------------------+
> +  // |     0 | V["hem", 0, 3]             |     0 | V["meh", 0, 3]             |

Should be `V["meh", 4, 3]` right?

::: modules/libpref/SharedPrefMap.h:310
(Diff revision 3)
> +    // the value arrays. Please see the documentation for the Value struct
> +    // above.
> +    Value mValue;
> +
> +    // The preference's type, as a PrefType enum value. This must *never* be
> +    // PrefType::None for values in a shared array.

Can you add an assert for this?

::: modules/libpref/SharedPrefMap.h:399
(Diff revision 3)
> +    Pref(const Pref& aPref) = default;
> +
> +  protected:
> +    friend class SharedPrefMap;
> +
> +    Pref(const SharedPrefMap& aPrefMap, const Entry& aEntry)

I'd prefer if this just took pointers directly, or maybe just store refs. I don't feel too strongly about this.

::: modules/libpref/SharedPrefMap.h:710
(Diff revision 3)
> +      bool mHasUserValue;
> +      ValueType mDefaultValue;
> +      ValueType mUserValue{};
> +    };
> +
> +    AutoTArray<Entry, 256> mUserEntries;

nit: maybe larger, my prefs.js has something like 274 entries.

::: modules/libpref/SharedPrefMap.h:731
(Diff revision 3)
> +    {
> +    }
> +
> +    StringTableEntry Add(const CharType* aKey)
> +    {
> +      mEntries.AppendElement(Entry{ mSize, uint32_t(strlen(aKey)), aKey });

I think you can just do:

```
const auto& entry = *mEntriesAppendElement(...);
```

::: modules/libpref/SharedPrefMap.h:752
(Diff revision 3)
> +      }
> +    }
> +
> +    uint32_t Count() const { return mEntries.Length(); }
> +
> +    uint32_t Size() const { return mSize * sizeof(ElemType); }

nit: maybe call this `ByteSize`?

::: modules/libpref/SharedPrefMap.h:835
(Diff revision 3)
> +        return { false, false };
> +    }
> +  }
> +
> +  UniqueStringTableBuilder<char> mKeyTable{ kExpectedPrefCount };
> +  StringTableBuilder<nsCStringHashKey, nsCString> mValueStringTable;

Can we choose better names than `mValueStringTable` and `mStringValueTable`? It's a little confusing :)

::: modules/libpref/SharedPrefMap.cpp:136
(Diff revision 3)
> +                          const Flags& aFlags,
> +                          const nsCString& aDefaultValue,
> +                          const nsCString& aUserValue)
> +{
> +  ValueIdx index;
> +  StringTableEntry defaultVal = mValueStringTable.Add(aDefaultValue);



::: modules/libpref/SharedPrefMap.cpp:225
(Diff revision 3)
> +  auto headerPtr = mem.Get<Header>();
> +  headerPtr[0] = header;
> +
> +  auto* entryPtr = reinterpret_cast<SharedPrefMap::Entry*>(&headerPtr[1]);
> +  for (auto* entry : entries) {
> +    *entryPtr++ = {

nit: please split the increment out
Attachment #8989851 - Flags: review?(erahm) → review-
Assignee

Comment 67

11 months ago
mozreview-review-reply
Comment on attachment 8989851 [details]
Bug 1471025: Part 2 - Add a helper class creating and accessing shared preference map snapshots.

https://reviewboard.mozilla.org/r/254816/#review262016

> Should be `V["meh", 4, 3]` right?

Oh. Yes.

> I'd prefer if this just took pointers directly, or maybe just store refs. I don't feel too strongly about this.

Yeah, it probably should, at this point. It started out storing refs, but you can't have a copy assignment operator for classes with reference members, so it made it impossible to use with a range iterator.

> nit: maybe larger, my prefs.js has something like 274 entries.

These are per type, one for int prefs and one for string prefs, so I think it should be fine for most users.

> Can we choose better names than `mValueStringTable` and `mStringValueTable`? It's a little confusing :)

I don't know, can we? I didn't like it either, but I also couldn't think of any better alternatives...
Assignee

Comment 68

11 months ago
(In reply to Nicholas Nethercote [:njn] from comment #56)
> > when you pass 4096 as the initial size to a
> > PLDHashTable, it allocates space for 8192 entries. When you pass 4095, it
> > allocates space for 4096.
> 
> That's not right. The constructor's comment says:
> 
> "The table's initial capacity is chosen such that |aLength| elements can be
> inserted without rehashing; if |aLength| is a power-of-two, this capacity
> will be |2*length|."
> 
> The capacity is chosen such that the given length makes the table between
> 25% and 75% full. So any length in the range 2048--6144 (or possibly
> 2049--6143... I'm not certain about the edge cases) will result in a
> capacity of 8192.

I removed this part of the comment, since these hashtables are temporary, so their size doesn't matter much, and this happens at startup, when we don't have to worry much about memory fragmentation. But I moved it to the constants for the content pref hashtable sizes, and clarified.
Assignee

Comment 69

11 months ago
mozreview-review-reply
Comment on attachment 8989851 [details]
Bug 1471025: Part 2 - Add a helper class creating and accessing shared preference map snapshots.

https://reviewboard.mozilla.org/r/254816/#review262016

> Can you add an assert for this?

There's already an assert in GetValue() when we serialize. I suppose adding an assert to Type() couldn't hurt.
Assignee

Comment 70

11 months ago
mozreview-review-reply
Comment on attachment 8989851 [details]
Bug 1471025: Part 2 - Add a helper class creating and accessing shared preference map snapshots.

https://reviewboard.mozilla.org/r/254816/#review261742

> This format is flexible enough that imagining it is a challenge. What would help immensely would be a small example involving a handful of prefs. Would you mind adding one?

Added one, but it's not exactly small...
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 83

11 months ago
Note: I rebased these on top of inbound so I could use the tri-state Sort() from bug 1457728 rather than NS_QuickSort(). The inter-diff might be a bit weird.

Comment 84

11 months ago
mozreview-review
Comment on attachment 8989851 [details]
Bug 1471025: Part 2 - Add a helper class creating and accessing shared preference map snapshots.

https://reviewboard.mozilla.org/r/254816/#review262380

::: modules/libpref/SharedPrefMap.cpp:168
(Diff revisions 3 - 4)
>    // using binary search.
>    nsTArray<Entry*> entries(mEntries.Length());
>    for (auto& entry : mEntries) {
>      entries.AppendElement(&entry);
>    }
> -  // Note: We can't currently use nsTArray::Sort here, since it requires a
> +  entries.Sort([](const Entry* aA, const Entry* aB) {

Much nicer!
Attachment #8989851 - Flags: review?(erahm) → review+
Assignee

Comment 85

11 months ago
Fun fact bug 1351200 replaced our original IsMainThread() assertions with:

  MOZ_ASSERT(NS_IsMainThread() || mozilla::ServoStyleSet::IsInServoTraversal());

so it could read prefs off the main thread when the main thread was supposed to be blocked. So the IsMainThread() assertion I added to pref_Lookup() failed in debug runs.

That exception seemed a bit sketchy, but I copied it to pref_Lookup(), which confirmed that it is indeed sketchy, because now the GetOrCreate call in AddAccessCount occasionally asserts when called from that background thread with:

[task 2018-07-10T05:24:50.602Z] 05:24:50     INFO - GECKO(1204) | Assertion failure: IsIdle(oldState), at /builds/worker/workspace/build/src/xpcom/ds/PLDHashTable.h:132

Which means that we're doing pref lookups on multiple threads at the same time, despite the that Servo's locking, which was supposed to make the exception safe, is meant to prevent that.


So I'm probably going to add an IsMainThread() check to the AddAccessCount call as a temporary workaround, and file a bug to get the stylo code fixed.
Assignee

Updated

11 months ago
No longer blocks: 1474163
Comment on attachment 8989852 [details]
Bug 1471025: Part 3a - Pass shared preference map to (non-Android) content processes at startup.

https://reviewboard.mozilla.org/r/254818/#review263726

::: modules/libpref/Preferences.h:75
(Diff revision 4)
> +// 2) A set of changes on top of the snapshot, containing the current values of
> +//    all preferences which have changed since it was created.
> +//
> +// Since the second set will be different for every process, and the first set
> +// cannot be modified, it is unfortunately not possible to combine them into a
> +// single file descriptor.

Thanks for commenting this.
Attachment #8989852 - Flags: review?(jld) → review+
Comment on attachment 8989853 [details]
Bug 1471025: Part 3b - Refactor Android shared FD API to require fewer modifications per change.

https://reviewboard.mozilla.org/r/254820/#review263716

::: commit-message-a4915:7
(Diff revision 4)
> +
> +Adding or removing an FD from this API currently requires changes in about a
> +half dozen places. Ignoring the Java side of things. This patch changes the
> +API to pass a struct, rather than additional arguments for each FD, so that
> +adding and removing FDs only requires changing one declaration, and the two
> +call sites that add and comsume the FDs.

Typo nit: consume
Attachment #8989853 - Flags: review?(jld) → review+
Comment on attachment 8989854 [details]
Bug 1471025: Part 3c - Also pass the shared preference map handle to Android content processes.

https://reviewboard.mozilla.org/r/254822/#review263718

::: dom/ipc/ContentProcess.h:53
(Diff revision 4)
>  
>    DISALLOW_EVIL_CONSTRUCTORS(ContentProcess);
>  };
>  
>  #ifdef ANDROID
> -// Android doesn't use -prefsHandle, it gets that FD another way.
> +// Android doesn't use -prefsHandle, it gets that FDs another way.

Typo nit: those FDs (also, there's another flag besides `-prefsHandle` now)
Attachment #8989854 - Flags: review?(jld) → review+
Assignee

Comment 89

11 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a8fbb472c91f13554cac3d0ea638cf9f368ff11
Bug 1471025: Part 1 - Store preference access counts in a separate hashtable. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/68bb03c63b3cee1d47cbddfd3abf919f5783c04b
Bug 1471025: Part 2 - Add a helper class creating and accessing shared preference map snapshots. r=njn,erahm

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3bbc87b71af2f2ce1fa8bdf2cf26857c071ba5e
Bug 1471025: Part 3a - Pass shared preference map to (non-Android) content processes at startup. r=jld,njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b7a8a35ed956159e2f443c6211164c0cbf3d926
Bug 1471025: Part 3b - Refactor Android shared FD API to require fewer modifications per change. r=jld

https://hg.mozilla.org/integration/mozilla-inbound/rev/38f690f30e78764763bb012045073fa781efa691
Bug 1471025: Part 3c - Also pass the shared preference map handle to Android content processes. r=jld

https://hg.mozilla.org/integration/mozilla-inbound/rev/83d98ea5ebaccded8a20929c0f3316e5618f1f76
Bug 1471025: Part 4 - Add a wrapper class that can access either static or dynamic prefs. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/d344247b870668f53fa645e72bda4bb4309346c8
Bug 1471025: Part 5 - Add a range iterator helper for iterating both static and dynamic preferences. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/faef4df47b2089592df7637f5b8f4ae193e98046
Bug 1471025: Part 6 - Optimize preference lookups while dispatching callbacks. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/5051f15fc2005667cfe76ccae0afb1fb0657c103
Bug 1471025: Part 7a - Look up preferences from both dynamic and shared preference tables. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc7ec17179d1961d91b897cec9f409786363ec9e
Bug 1471025: Part 7b - Don't load preference files in the content process. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/599895de063ef005dbd34847db9ee555410a878f
Bug 1471025: Part 7c - Clear the dynamic hashtable in the parent process after snapshotting. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/398ccedc20dc1c3f29332dd5a4791b9ab96eb547
Bug 1471025: Part 8 - Add tests for shared memory preferences. r=njn
Assignee

Comment 91

11 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c490838c8329f6b0c21fa57ef078c44bf7a9ba8d
Bug 1471025: Part 1 - Store preference access counts in a separate hashtable. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/434106f1b75e3ba900912f261bd22a1b7f5c931d
Bug 1471025: Part 2 - Add a helper class creating and accessing shared preference map snapshots. r=njn,erahm

https://hg.mozilla.org/integration/mozilla-inbound/rev/46a6f9d0773ba2d756d8801cee79bfa994165d44
Bug 1471025: Part 3a - Pass shared preference map to (non-Android) content processes at startup. r=jld,njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff41551f5ff1b98b72ed771a6f2a3f66a8b79a57
Bug 1471025: Part 3b - Refactor Android shared FD API to require fewer modifications per change. r=jld

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c03b7dd00e9675f9ac045ed1ea733eb0486904f
Bug 1471025: Part 3c - Also pass the shared preference map handle to Android content processes. r=jld

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce379eaab17905f39f1665c3e40f683ebd3f8824
Bug 1471025: Part 4 - Add a wrapper class that can access either static or dynamic prefs. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9362cf1add47c2f62529e42764ed6088d274170
Bug 1471025: Part 5 - Add a range iterator helper for iterating both static and dynamic preferences. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/8eff817d2f7e07409269899c048a9091220dec07
Bug 1471025: Part 6 - Optimize preference lookups while dispatching callbacks. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/b4f9178f132de2b5f7064df9a9e1b489ea6576c3
Bug 1471025: Part 7a - Look up preferences from both dynamic and shared preference tables. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f4cc96fae1212cb2220770ac7311b9cc51af744
Bug 1471025: Part 7b - Don't load preference files in the content process. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c72dc1bff88e81f6c754b0971a44b9592d17ee1
Bug 1471025: Part 7c - Clear the dynamic hashtable in the parent process after snapshotting. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/200bec7e766a7937e71a4805083fb71e16c5e111
Bug 1471025: Part 8 - Add tests for shared memory preferences. r=njn
Assignee

Comment 92

11 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b672d70f335de6d4d39136ca51b41211666dc42
Bug 1471025: Follow-up: Bump pref access expectations for nonexistent preferences. r=bustage,test-only CLOSED TREE
Assignee

Comment 94

11 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/db184dff59238730c6b1c7d7bef85e96ec479f12
Bug 1471025: Part 1 - Store preference access counts in a separate hashtable. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc3c747109c28f901a61b9add8a45e08b57e28c4
Bug 1471025: Part 2 - Add a helper class creating and accessing shared preference map snapshots. r=njn,erahm

https://hg.mozilla.org/integration/mozilla-inbound/rev/97f4e15738bc720693ae9c67acae7411f8223383
Bug 1471025: Part 3a - Pass shared preference map to (non-Android) content processes at startup. r=jld,njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/14e3a4118809564513576000048ad6806065e62d
Bug 1471025: Part 3b - Refactor Android shared FD API to require fewer modifications per change. r=jld

https://hg.mozilla.org/integration/mozilla-inbound/rev/a6971931a090c9ebbaa996362db24f33236ad5fb
Bug 1471025: Part 3c - Also pass the shared preference map handle to Android content processes. r=jld

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f8e876b2c98548c347406e9aa9b57885a49c2a1
Bug 1471025: Part 4 - Add a wrapper class that can access either static or dynamic prefs. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/05486bb725e8063b73bc6dc0ebc75e8c543e39ab
Bug 1471025: Part 5 - Add a range iterator helper for iterating both static and dynamic preferences. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/a52838ca98e194b5e64c93458b7f3baa30aca33c
Bug 1471025: Part 6 - Optimize preference lookups while dispatching callbacks. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce68155dc6eb48c78769fa7dc13704221f0db2a6
Bug 1471025: Part 7a - Look up preferences from both dynamic and shared preference tables. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6cb808b74bf937dd710310a3860ea4456d57662
Bug 1471025: Part 7b - Don't load preference files in the content process. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a685c1dc046cb3005c9b7a9f874b5fd935f2bb1
Bug 1471025: Part 7c - Clear the dynamic hashtable in the parent process after snapshotting. r=njn

https://hg.mozilla.org/integration/mozilla-inbound/rev/4eaf01f13fbcdf3d99cfc96da10c79f577f3df25
Bug 1471025: Part 8 - Add tests for shared memory preferences. r=njn
Assignee

Comment 95

11 months ago
Over all, the new startup, modification, and synchronization flows look like this:

Parent process startup:
-----------------------

Initially, has only a dynamic hash table.

Early in startup, loads all of the static preferences and built-in preference
files (mainly from omni.ja) into that hash table.

It also registers variable caches for static preferences, stores their initial
default values in those caches, and registers callbacks to update them as
necessary.

Slightly later in startup, loads all user preference files (mainly from the
profile directory).

Any preferences changed at this point will trigger change callbacks for
matching variable caches, and the cached values will be updated as
appropriate.


As soon as we're ready to launch a content process, the full state of the hash
table is serialized to a snapshot, which is stored in a shared memory region
managed by a SharedPrefMap instance. The original hash table is then cleared,
and used only to store changed preference values.


Content process startup, parent side:
-------------------------------------

When the parent is ready to start a content process, it first ensures that the
initial preference snapshot exists, and adds its file descriptor (`prefMapFd`)
to the initial file descriptor set for the content processes. All content
processes receive the exact same snapshot.

It then serializes all preference values in the dynamic hash table, stores
them in a separate shared memory region, and adds its file descriptor
(`changedPrefsFd`) to the initial set for the process. This represents the set
of changes the content process needs to apply on top of the snapshot, and
allows it to build a dynamic hash table which should exactly match the
parent's. This second shared memory region is not write protected, and each
content process receives a completely separate copy.


Content process startup, content side:
--------------------------------------

Early in startup, the preference service maps and deserializes both shared
memory regions sent from the parent, but defers further initialization until
requested by XPCOM init.

When the preference service is initialized, variable caches are initialized
for static preferences, but no default values are set, and no preference files
are loaded.

Once the variable caches have been initialized, we dispatch preference change
callbacks for any preferences in the shared snapshot which have user values or
are locked. This allows the variable caches to update their cached values.

After that, the changed preference values received from the parent process
(via `changedPrefsFd`) are added to the database. Their values override the
values in the snapshot, and preference change callbacks are dispatched for
them as appropriate.


Preference lookups:
-------------------

Each preference database consists of two maps: a dynamic hash table, and a
shared memory snapshot. A given preference may have an entry in either or both
of the two maps, but only one entry entry is used. If a preference exists in
both maps, the dynamic entry takes precedence.

For preference lookups, the dynamic hash table is checked first, followed by
the shared snapshot. The first entry found is returned. The entry in the
dynamic hash table *may* have the type None, in which case the preference is
treated as if it did not exist. The preference in the static snapshot never
has the type None.

For preference enumeration, both maps are enumerated, starting with the
dynamic hash table. While iterating over the dynamic hash table, any entry
with the type None is skipped. While iterating over the shared snapshot, any
entry which also exists in the dynamic hash table is skipped. The combined
result of the two iterations represents the full contents of the preference
database.


Preference changes:
-------------------

Preference changes before the initial snapshot have been made are simple.
Entries for the preferences are simply looked up or created in the dynamic
preference hash table, and updated as appropriate. There is no shared snapshot
to worry about overriding, and no content processes to worry about
synchronizing with.


Once a snapshot has been created, any changes need to happen in the dynamic
hash table.

If an entry for a changed preference already exists in the dynamic hash table,
that entry can simply be changed to reflect any new preference changes.
Likewise for preferences that do not exist in either the dynamic hash table or
the shared snapshot: A new dynamic entry can simply be created and populated
as necessary.

If an entry for a changed preference exists in the snapshot but not in the
dynamic hash table, we need to be more careful. In those cases, we need to
create a dynamic entry with the same values as the snapshot entry, and update
it with our changes. But *only* if the changes will have an effect. If a
caller attempts to set a preference to its existing value, we do not want to
waste memory creating a dynamic entry which is essentially unused.


In any of these cases, if a preference value changes, we need to communicate
that change to the child process. This synchronization is much simpler: We
only care about changes which have a *visible* effect. A change to a default
value which is hidden by a user value is unimportant.

We handle this synchronization by notifying preference observers whenever the
visible value of a preference changes. A preference observer then takes a
snapshot of the full state of the changed preference and broadcasts it to each
child process.


Preference deletions:
---------------------

Deleting preferences is slightly more complicated. If a preference to be
deleted exists only in the dynamic hash table, its entry can simply be
removed. If it exists in the shared snapshot, however, its dynamic hash table
entry needs to be kept (or created), and its type changed to None. The
presence of this entry masks the snapshot entry, and is ignored by preference
enumerators.
Flags: needinfo?(kmaglione+bmo)
Assignee

Comment 96

10 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7dc962b45c0a41a841b2fa2bb08b82cd6a13f90a
Bug 1471025: Follow-up: Bump pref access expectations for nonexistent preferences again. r=bustage,test-only DONTBUILD

Updated

10 months ago
Depends on: 1475836
Perf wins:

== Change summary for alert #14349 (as of Fri, 13 Jul 2018 18:15:28 GMT) ==

Improvements:

  5%  Base Content Explicit windows7-32 pgo stylo     10,468,010.67 -> 9,968,469.33
  2%  Base Content Explicit windows10-64 opt stylo    15,491,072.00 -> 15,140,864.00

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=14349
Depends on: 1477254
Assignee

Updated

10 months ago
Depends on: 1477904
Depends on: 1502410
Depends on: 1505941
You need to log in before you can comment on or make changes to this bug.