1348959 - Remove wraparound indexing in ProfileBuffer

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement)

Description

[Julian Seward [:jseward]]

ProfileBuffer implements a circular buffer for samples.  It maintains start
and end pointers for the live section in mReadPos and mWritePos respectively,
and always keeps one slot unused.

Although seemingly simple, this causes hard-to-reason about wraparound cases
in various places, most notably in ProfileBuffer::FindLastSampleOfThread as
revised by bug 1345032, and in ProfileBuffer::reset.  There are also integer
mod operations in various loops in ProfileBufferEntry.cpp, which I suspect are
beginning to show up as a significant expense as the profiler's performance
improves.

Here's an alternative proposal:

* Conceptually, give each sample a unique unsigned 64 bit number.  These are
  never reused.

* Change ProfileBuffer::{mReadPos, mWritePos} to be unsigned 64 bit ints.

* Require all buffer sizes to be a power of two.  This doesn't strike me as a
  big loss.

* Computation of array entry numbers is then cheap -- AND with mask.

The effects are:

* The only invariants we need to maintain are
     mWritePos >= mReadPos
     mWritePos - mReadPos <= mEntrySize

* mGeneration can be removed.  We can empty out the buffer by setting
  mReadPos = mWritePos.

* Scans over the buffer (eg DuplicateLastSample) don't have to be circular.
  They can just be of the form
    for (uint64_t ix = mReadPos; ix < mWritePos; ix++) ...

* It is trivial to decide if a sample is still in the buffer -- just compare
  its index with mReadPos.  This would make FindLastSampleOfThread's
  correctness easier to reason about.

Note that we are already in effect using mGeneration to expand the buffer's
address space beyond one buffer cycle.  But it is used in such a way that
reasoning is difficult.  For example, it's not clear whether or not there's a
total ordering on (generation, index) pairs, and we can't cheaply subtract
such pairs so as to find out how far apart two entries are.  The above
proposal would fix both of those.

Comment 1

[Markus Stange [:mstange]]

Sounds good to me. So this would no longer keep one slot unused? I hope we're not relying on that anywhere.

Comment 2

[Markus Stange [:mstange]]

I've started working on this. I didn't limit mEntrySize to be a power two for now, I just added modulo operations everywhere.

Comment 3

[Julian Seward [:jseward]]

(In reply to Markus Stange [:mstange] from comment #2)
> I've started working on this.

Nice.

> I didn't limit mEntrySize to be a power two for now, I just added
> modulo operations everywhere.

But you do intend to move eventually to a power-of-2-with-masking
scheme, yes?  Given that 64-bit integer % is going to cost ~32 cycles
per operation if the machine can provide 2 bits/cycle, and way
more than that on a 32-bit target.

Comment 4

[Markus Stange [:mstange]]

Attachment: Bug 1348959 - Make ProfileBuffer fields uint32_t.

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

Comment 5

[Markus Stange [:mstange]]

Attachment: Bug 1348959 - Make profiler_get_buffer_info() return information in a struct instead of using outparams.

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

Comment 6

[Markus Stange [:mstange]]

Attachment: Bug 1348959 - Remove wraparound indexing in ProfileBuffer.

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

Comment 7

[Markus Stange [:mstange]]

(In reply to Julian Seward [:jseward] from comment #3)
> > I didn't limit mEntrySize to be a power two for now, I just added
> > modulo operations everywhere.
> 
> But you do intend to move eventually to a power-of-2-with-masking
> scheme, yes?  Given that 64-bit integer % is going to cost ~32 cycles
> per operation if the machine can provide 2 bits/cycle, and way
> more than that on a 32-bit target.

Thanks for reminding me of the costs. I've now done what you initially suggested.

Comment 8

[Markus Stange [:mstange]]

The third patch is rather big, unfortunately.
I'm asking Kannan to review the js/ parts, and Nika to review the tools/profiler/ parts.

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 8945919 [details]
Bug 1348959 - Make ProfileBuffer fields uint32_t.

https://reviewboard.mozilla.org/r/215998/#review221902

Stealing review; I suspect Nika won't mind :)

::: tools/profiler/core/ProfileBufferEntry.cpp:1040
(Diff revision 1)
>  bool
>  ProfileBuffer::DuplicateLastSample(int aThreadId,
>                                     const TimeStamp& aProcessStartTime,
>                                     LastSample& aLS)
>  {
> -  int lastSampleStartPos = FindLastSampleOfThread(aThreadId, aLS);
> +  Maybe<uint32_t> lastSampleStart = FindLastSampleOfThread(aThreadId, aLS);

Call this `maybeLastSampleStartPos`?

::: tools/profiler/core/platform.cpp:2737
(Diff revision 1)
>  #if defined(GP_PLAT_amd64_windows)
>    InitializeWin64ProfilerHooks();
>  #endif
>  
>    // Fall back to the default values if the passed-in values are unreasonable.
> -  int entries = aEntries > 0 ? aEntries : PROFILER_DEFAULT_ENTRIES;
> +  uint32_t entries = aEntries > 0 ? aEntries : PROFILER_DEFAULT_ENTRIES;

Should `aEntries` be made uint32_t as well?

Comment 10

[Nicholas Nethercote [inactive]]

Comment on attachment 8945920 [details]
Bug 1348959 - Make profiler_get_buffer_info() return information in a struct instead of using outparams.

https://reviewboard.mozilla.org/r/216000/#review221904

Nice change.

::: tools/profiler/public/GeckoProfiler.h:416
(Diff revision 1)
>  
>  // Immediately capture the current thread's call stack and return it. A no-op
>  // if the profiler is inactive or in privacy mode.
>  UniqueProfilerBacktrace profiler_get_backtrace();
>  
> -// Get information about the current buffer status. A no-op when the profiler
> +struct ProfilerBufferInfo {

Curly brace should be on its own line.

::: tools/profiler/public/GeckoProfiler.h:417
(Diff revision 1)
>  // Immediately capture the current thread's call stack and return it. A no-op
>  // if the profiler is inactive or in privacy mode.
>  UniqueProfilerBacktrace profiler_get_backtrace();
>  
> -// Get information about the current buffer status. A no-op when the profiler
> -// is inactive. Do not call this function; call profiler_get_buffer_info()
> +struct ProfilerBufferInfo {
> +  uint32_t writePosition;

These should all have the `mFoo` form.

::: tools/profiler/tests/gtest/GeckoProfiler.cpp:196
(Diff revision 1)
>      // Active -> Active with same settings
>  
>      // First, write some samples into the buffer.
>      PR_Sleep(PR_MillisecondsToInterval(500));
>  
> -    uint32_t currPos1, entries1, generation1;
> +    auto bufferState1 = *profiler_get_buffer_info();

This use of `auto` doesn't fit the style guide's recommended uses, which are mostly in cases where the type would be written twice. Please use `Maybe<ProfileBufferInfo>` instead.

You could shorten the var names to `info1` and `info2`, though.

::: tools/profiler/tests/gtest/GeckoProfiler.cpp:209
(Diff revision 1)
>      ActiveParamsCheck(PROFILER_DEFAULT_ENTRIES, PROFILER_DEFAULT_INTERVAL,
>                        features, filters, MOZ_ARRAY_LENGTH(filters));
>  
>      // Check that our position in the buffer stayed the same or advanced.
>      // In particular, it shouldn't have reverted to the start.
> -    uint32_t currPos2, entries2, generation2;
> +    auto bufferState2 = *profiler_get_buffer_info();

Ditto

::: tools/profiler/tests/gtest/GeckoProfiler.cpp:218
(Diff revision 1)
>    }
>  
>    {
>      // Active -> Active with *different* settings
>  
> -    uint32_t currPos1, entries1, generation1;
> +    auto bufferState1 = *profiler_get_buffer_info();

Ditto

::: tools/profiler/tests/gtest/GeckoProfiler.cpp:231
(Diff revision 1)
>                              filters, MOZ_ARRAY_LENGTH(filters));
>  
>      ActiveParamsCheck(PROFILER_DEFAULT_ENTRIES, PROFILER_DEFAULT_INTERVAL,
>                        differentFeatures, filters, MOZ_ARRAY_LENGTH(filters));
>  
> -    uint32_t currPos2, entries2, generation2;
> +    auto bufferState2 = *profiler_get_buffer_info();

Ditto

::: tools/profiler/tests/gtest/GeckoProfiler.cpp:384
(Diff revision 1)
>  
> -  uint32_t currPos1, entries1, generation1;
> -  uint32_t currPos2, entries2, generation2;
> -
>    // Check that we are writing samples while not paused.
> -  profiler_get_buffer_info(&currPos1, &entries1, &generation1);
> +  auto bufferState1 = *profiler_get_buffer_info();

Ditto

::: tools/profiler/tests/gtest/GeckoProfiler.cpp:386
(Diff revision 1)
> -  uint32_t currPos2, entries2, generation2;
> -
>    // Check that we are writing samples while not paused.
> -  profiler_get_buffer_info(&currPos1, &entries1, &generation1);
> +  auto bufferState1 = *profiler_get_buffer_info();
>    PR_Sleep(PR_MillisecondsToInterval(500));
> -  profiler_get_buffer_info(&currPos2, &entries2, &generation2);
> +  auto bufferState2 = *profiler_get_buffer_info();

Ditto

::: tools/profiler/tests/gtest/GeckoProfiler.cpp:394
(Diff revision 1)
>    profiler_pause();
>  
>    ASSERT_TRUE(profiler_is_paused());
>  
>    // Check that we are not writing samples while paused.
> -  profiler_get_buffer_info(&currPos1, &entries1, &generation1);
> +  bufferState1 = *profiler_get_buffer_info();

Ditto

::: tools/profiler/tests/gtest/GeckoProfiler.cpp:396
(Diff revision 1)
>    ASSERT_TRUE(profiler_is_paused());
>  
>    // Check that we are not writing samples while paused.
> -  profiler_get_buffer_info(&currPos1, &entries1, &generation1);
> +  bufferState1 = *profiler_get_buffer_info();
>    PR_Sleep(PR_MillisecondsToInterval(500));
> -  profiler_get_buffer_info(&currPos2, &entries2, &generation2);
> +  bufferState2 = *profiler_get_buffer_info();

Ditto

Comment 11

[Markus Stange [:mstange]]

Comment on attachment 8945921 [details]
Bug 1348959 - Remove wraparound indexing in ProfileBuffer.

https://reviewboard.mozilla.org/r/216002/#review221928

::: tools/profiler/core/ProfileBuffer.cpp:26
(Diff revision 1)
> +  size_t entrySize = RoundUpPow2(aEntrySize);
> +  MOZ_RELEASE_ASSERT(entrySize <= size_t(UINT32_MAX),
> +                     "aEntrySize is larger than what we support");

I realized that this check doesn't work in 32 bit builds. I'm going to change it to
  const uint32_t UINT32_MAX_POWER_OF_TWO = 1 << 31;
  MOZ_RELEASE_ASSERT(aEntrySize <= UINT32_MAX_POWER_OF_TWO,
                     "aEntrySize is larger than what we support");

Comment 12

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8945919 [details]
Bug 1348959 - Make ProfileBuffer fields uint32_t.

https://reviewboard.mozilla.org/r/215998/#review222074

Don't have much to add over what njn has already said :-)

Comment 13

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8945920 [details]
Bug 1348959 - Make profiler_get_buffer_info() return information in a struct instead of using outparams.

https://reviewboard.mozilla.org/r/216000/#review222078

::: tools/profiler/gecko/nsProfiler.cpp:503
(Diff revision 1)
> +  if (info) {
> +    *aCurrentPosition = info->writePosition;
> +    *aTotalSize = info->entryCount;
> +    *aGeneration = info->generation;
> +  } else {
> +    *aCurrentPosition = 0;

Feels like it might be clearer to make `profiler_get_buffer_info` just construct a ProfilerBufferInto filled with 0s when the profiler isn't active, as I don't see any other consumers which consider the `Nothing` case.

Comment 14

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8945921 [details]
Bug 1348959 - Remove wraparound indexing in ProfileBuffer.

https://reviewboard.mozilla.org/r/216002/#review222082

::: js/src/jit/JitcodeMap.h:185
(Diff revision 1)
> -        void setGeneration(uint32_t gen) {
> -            gen_ = gen;
> +        void setSamplePositionInBuffer(uint64_t bufferWritePos) {
> +            samplePositionInBuffer_ = bufferWritePos;
>          }
> -        bool isSampled(uint32_t currentGen, uint32_t lapCount) {
> -            if (gen_ == UINT32_MAX || currentGen == UINT32_MAX)
> +        bool isSampled(uint64_t bufferRangeStart) {
> +            if (samplePositionInBuffer_ == UINT64_MAX || bufferRangeStart == UINT64_MAX)
>                  return false;

nit: add curlys?

::: js/src/vm/Runtime.h:368
(Diff revision 1)
> -        return profilerSampleBufferLapCount_;
> -    }
> -    void resetProfilerSampleBufferLapCount() {
> -        profilerSampleBufferLapCount_ = 1;
>      }
> -    void updateProfilerSampleBufferLapCount(uint32_t lapCount) {
> +    void resetProfilerSampleBufferRangeStart() {

Why not just call setProfilerSampleBufferRangeStart(0)?

::: tools/profiler/core/platform.cpp:741
(Diff revision 1)
>    // need to iterate oldest-to-youngest when adding entries to aInfo.
>  
> -  // Synchronous sampling reports an invalid buffer generation to
> -  // ProfilingFrameIterator to avoid incorrectly resetting the generation of
> -  // sampled JIT entries inside the JS engine. See note below concerning 'J'
> -  // entries.
> +  // Synchronous sampling reports an invalid buffer write position to
> +  // ProfilingFrameIterator to avoid incorrectly resetting the buffer position
> +  // of sampled JIT entries inside the JS engine.
> +  uint64_t samplePosInBuffer = UINT64_MAX;

Can you change most references to UINT64_MAX to instead refer to a constant like "ProfileBuffer::kInvalidEntry" or something like that?

::: tools/profiler/core/platform.cpp:742
(Diff revision 1)
>  
> -  // Synchronous sampling reports an invalid buffer generation to
> -  // ProfilingFrameIterator to avoid incorrectly resetting the generation of
> -  // sampled JIT entries inside the JS engine. See note below concerning 'J'
> -  // entries.
> -  uint32_t startBufferGen = UINT32_MAX;
> +  // Synchronous sampling reports an invalid buffer write position to
> +  // ProfilingFrameIterator to avoid incorrectly resetting the buffer position
> +  // of sampled JIT entries inside the JS engine.
> +  uint64_t samplePosInBuffer = UINT64_MAX;
> +  if (!aIsSynchronous) {

aIsSynchronous is false when in `profiler_suspend_and_sample_thread` called from BHR. We probably don't want to reset the buffer position of sampled JIT entries inside the JS engine in that situation either (that method also uses a separate buffer).

::: tools/profiler/core/platform.cpp:908
(Diff revision 1)
>    // Do not do this for synchronous samples, which use their own
>    // ProfileBuffers instead of the global one in CorePS.

Maybe mention that BufferRangeStart() is null for profiler_suspend_and_sample_thread() which means that this code won't be run in that case, because we definitely don't want to update the JS engine in that case.

Perhaps we want to add a method to aCollector like "IsPeriodicSample"?

::: tools/profiler/core/platform.cpp:910
(Diff revision 1)
>  
>    // Update the JS context with the current profile sample buffer generation.
>    //
>    // Do not do this for synchronous samples, which use their own
>    // ProfileBuffers instead of the global one in CorePS.
> -  if (!aIsSynchronous && context && aCollector.Generation().isSome()) {
> +  if (!aIsSynchronous && context && aCollector.BufferRangeStart().isSome()) {

I think you can probably get away without the `.isSome()` call due to `Maybe`s `operator bool`.

::: tools/profiler/gecko/nsProfiler.cpp:499
(Diff revision 1)
>    MOZ_ASSERT(aCurrentPosition);
>    MOZ_ASSERT(aTotalSize);
>    MOZ_ASSERT(aGeneration);
>    Maybe<ProfilerBufferInfo> info = profiler_get_buffer_info();
>    if (info) {
> -    *aCurrentPosition = info->writePosition;
> +    *aCurrentPosition = info->rangeEnd % info->entryCount;

Should we assert that info->entryCount is a power of two and use the mask here?

This is almost certainly fast enough so it's not a big deal.

Comment 15

[Markus Stange [:mstange]]

Comment on attachment 8945919 [details]
Bug 1348959 - Make ProfileBuffer fields uint32_t.

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

Comment 16

[Markus Stange [:mstange]]

Comment on attachment 8945920 [details]
Bug 1348959 - Make profiler_get_buffer_info() return information in a struct instead of using outparams.

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

Comment 17

[Markus Stange [:mstange]]

Comment on attachment 8945921 [details]
Bug 1348959 - Remove wraparound indexing in ProfileBuffer.

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

Comment 18

[Markus Stange [:mstange]]

Comment on attachment 8945919 [details]
Bug 1348959 - Make ProfileBuffer fields uint32_t.

https://reviewboard.mozilla.org/r/215998/#review221902

> Call this `maybeLastSampleStartPos`?

I've renamed it, but part 3 removes this variable anyway.

> Should `aEntries` be made uint32_t as well?

Good call. I thought that doing this would cause more ripple effects, but it turns out that all callers of profiler_start already use a uint32_t for this, so it was easy to make that change.

Comment 19

[Code Review Bot [:reviewbot]]

Comment on attachment 8945921 [details]
Bug 1348959 - Remove wraparound indexing in ProfileBuffer.

https://reviewboard.mozilla.org/r/216002/#review222562


Static analysis found 2 defects in this patch.
 - 2 defects found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: js/src/vm/Stack.cpp:1904
(Diff revision 2)
>      activation_ = activation_->prev();
>      return *this;
>  }
>  
>  JS::ProfilingFrameIterator::ProfilingFrameIterator(JSContext* cx, const RegisterState& state,
> -                                                   uint32_t sampleBufferGen)
> +                                                   Maybe<uint64_t> samplePositionInProfilerBuffer)

Error: Type 'maybe<uint64_t>' (aka 'maybe<unsigned long>') must not be used as parameter [clang-tidy: mozilla-non-memmovable-template-arg]

::: js/src/vm/Stack.cpp:1906
(Diff revision 2)
>  }
>  
>  JS::ProfilingFrameIterator::ProfilingFrameIterator(JSContext* cx, const RegisterState& state,
> -                                                   uint32_t sampleBufferGen)
> +                                                   Maybe<uint64_t> samplePositionInProfilerBuffer)
>    : cx_(cx),
> -    sampleBufferGen_(sampleBufferGen),
> +    samplePositionInProfilerBuffer_(samplePositionInProfilerBuffer),

Warning: Parameter 'samplepositioninprofilerbuffer' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

Comment 20

[Markus Stange [:mstange]]

Comment on attachment 8945920 [details]
Bug 1348959 - Make profiler_get_buffer_info() return information in a struct instead of using outparams.

https://reviewboard.mozilla.org/r/216000/#review221904

> These should all have the `mFoo` form.

I usually don't follow this style for pure data-storage structs whose fields are only accessed from the outside... but I've made the change anyway.

Comment 21

[Markus Stange [:mstange]]

Comment on attachment 8945920 [details]
Bug 1348959 - Make profiler_get_buffer_info() return information in a struct instead of using outparams.

https://reviewboard.mozilla.org/r/216000/#review222078

> Feels like it might be clearer to make `profiler_get_buffer_info` just construct a ProfilerBufferInto filled with 0s when the profiler isn't active, as I don't see any other consumers which consider the `Nothing` case.

(Nika withdrew this review comment in bug 1431179 comment 11, because the patch in that bug also uses the Nothing() return value to check whether the profiler is running.)

Comment 22

[Markus Stange [:mstange]]

Comment on attachment 8945921 [details]
Bug 1348959 - Remove wraparound indexing in ProfileBuffer.

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

Comment 23

[Markus Stange [:mstange]]

Comment on attachment 8945921 [details]
Bug 1348959 - Remove wraparound indexing in ProfileBuffer.

https://reviewboard.mozilla.org/r/216002/#review222082

> nit: add curlys?

Doesn't seem to be the local style.

> Why not just call setProfilerSampleBufferRangeStart(0)?

Done.

> Can you change most references to UINT64_MAX to instead refer to a constant like "ProfileBuffer::kInvalidEntry" or something like that?

I've changed most of the UINT64_MAX magic values to Nothing()s by putting Maybes everywhere except on BaseEntry. On BaseEntry I'm now using a constant called kNoSampleInBuffer.

> Maybe mention that BufferRangeStart() is null for profiler_suspend_and_sample_thread() which means that this code won't be run in that case, because we definitely don't want to update the JS engine in that case.
> 
> Perhaps we want to add a method to aCollector like "IsPeriodicSample"?

I added a comment. I don't think IsPeriodicSample would simplify anything.

> Should we assert that info->entryCount is a power of two and use the mask here?
> 
> This is almost certainly fast enough so it's not a big deal.

Yeah, I don't think that's necessary.

Comment 24

[Nicholas Nethercote [inactive]]

Comment on attachment 8945921 [details]
Bug 1348959 - Remove wraparound indexing in ProfileBuffer.

https://reviewboard.mozilla.org/r/216002/#review222596

Thank you for doing this. I never liked the generation stuff!

::: js/src/jit/JitcodeMap.h:146
(Diff revision 3)
>      typedef Vector<BytecodeLocation, 0, SystemAllocPolicy> BytecodeLocationVector;
>      typedef Vector<const char*, 0, SystemAllocPolicy> ProfileStringVector;
>  
>      struct BaseEntry
>      {
> +        static const uint64_t kNoSampleInBuffer = UINT64_MAX;

Could you use Maybe<> instead?

::: tools/profiler/core/ProfileBuffer.h:78
(Diff revision 3)
>    void Reset();
>  
> +  // Access an entry in the buffer.
> +  ProfileBufferEntry& GetEntry(uint64_t aPosition) const
> +  {
> +    return mEntries[aPosition & mEntryIndexMask];

Is it possible to encapsulate mEntries somehow so that it's difficult or impossible to accidentally access it directly? That would be reassuring.

::: tools/profiler/core/ProfileBuffer.h:92
(Diff revision 3)
> -  // we need to stop reading.
> +  uint32_t mEntryIndexMask;
> -  uint32_t mWritePos;
>  
> -  // Points to the entry at which we can start reading.
> -  uint32_t mReadPos;
> +public:
> +  // mRangeStart and mRangeEnd are uint64_t values that strictly advance and
> +  // never wrap around. mRangeEnd is always in front of mRangeStart, but never

"in front of" suggests "greater than", which isn't true. Change to "greater than or equal"?

::: tools/profiler/core/ProfileBuffer.h:101
(Diff revision 3)
> +  // buffer (and increase mRangeStart).
> +  // In other words, the following conditions hold true at all times:
> +  //  (1) mRangeStart <= mRangeEnd
> +  //  (2) mRangeEnd - mRangeStart <= mEntrySize
> +  // All accesses to entries in mEntries need to go through GetEntry(), which
> +  // translates the given buffer position from the near-infinite uint64_t space

"near-infinite", lol

::: tools/profiler/core/ProfileBuffer.h:106
(Diff revision 3)
> +  // translates the given buffer position from the near-infinite uint64_t space
> +  // into the entry storage space.
> +  // New entries are always added at the end of the buffer: mRangeEnd is also
> +  // the position at which the next entry will be written.
> +  uint64_t mRangeStart;
> +  uint64_t mRangeEnd;

This comment doesn't really state what mRangeStart and mRangeEnd mean. I think it would be helpful to include these descriptions in the comment:

- If there are no live entries, then mRangeStart==mRangeEnd.
- Otherwise, mRangeStart is the first live entry and mRangeEnd is one past the last live entry.
- (mRangeEnd - mRangeStart) always gives the number of live entries.

::: tools/profiler/core/ProfileBuffer.cpp:116
(Diff revision 3)
>  }
>  
>  void
>  ProfileBuffer::Reset()
>  {
> -  mGeneration += 2;
> +  mRangeStart = mRangeEnd = mRangeEnd + 2 * mEntrySize;

This needs a comment.

::: tools/profiler/gecko/nsProfiler.cpp:501
(Diff revision 3)
>    MOZ_ASSERT(aGeneration);
>    Maybe<ProfilerBufferInfo> info = profiler_get_buffer_info();
>    if (info) {
> -    *aCurrentPosition = info->mWritePosition;
> +    *aCurrentPosition = info->mRangeEnd % info->mEntryCount;
>      *aTotalSize = info->mEntryCount;
> -    *aGeneration = info->mGeneration;
> +    *aGeneration = info->mRangeEnd / info->mEntryCount;

Yikes, this still exists... can it be removed or changed?

Comment 25

[Kannan Vijayan [:djvj]]

Comment on attachment 8945921 [details]
Bug 1348959 - Remove wraparound indexing in ProfileBuffer.

https://reviewboard.mozilla.org/r/216002/#review223418

Thanks for the cleanup!  Much nicer approach.

Comment 26

[Markus Stange [:mstange]]

Comment on attachment 8945921 [details]
Bug 1348959 - Remove wraparound indexing in ProfileBuffer.

https://reviewboard.mozilla.org/r/216002/#review222596

> Could you use Maybe<> instead?

I tried to make this change but couldn't, because BaseEntry is used as a variant in a union and is not allowed to have a non-trivial destructor. Giving it a Maybe<> member would give it a non-trivial destructor. (Not that the destructor of a Maybe<uint64_t> really does anything useful...)

> Is it possible to encapsulate mEntries somehow so that it's difficult or impossible to accidentally access it directly? That would be reassuring.

mEntries is private; I've added another comment to it. In the future I'd like to extract the parts that are not specific to the profiler into a FixedCapacityQueue<T>, which would encapsulate this a bit better.

> "in front of" suggests "greater than", which isn't true. Change to "greater than or equal"?

Will do.

> This comment doesn't really state what mRangeStart and mRangeEnd mean. I think it would be helpful to include these descriptions in the comment:
> 
> - If there are no live entries, then mRangeStart==mRangeEnd.
> - Otherwise, mRangeStart is the first live entry and mRangeEnd is one past the last live entry.
> - (mRangeEnd - mRangeStart) always gives the number of live entries.

Good points, I've added those comments.

> Yikes, this still exists... can it be removed or changed?

I can file a bug, but it's not really causing much harm. The only user is going to be removed once the devtools profiler gets replaced.

Comment 27

[Markus Stange [:mstange]]

Comment on attachment 8945919 [details]
Bug 1348959 - Make ProfileBuffer fields uint32_t.

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

Comment 28

[Markus Stange [:mstange]]

Comment on attachment 8945920 [details]
Bug 1348959 - Make profiler_get_buffer_info() return information in a struct instead of using outparams.

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

Comment 29

[Markus Stange [:mstange]]

Comment on attachment 8945921 [details]
Bug 1348959 - Remove wraparound indexing in ProfileBuffer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216002/diff/3-4/

Comment 30

[Nicholas Nethercote [inactive]]

Comment on attachment 8945921 [details]
Bug 1348959 - Remove wraparound indexing in ProfileBuffer.

https://reviewboard.mozilla.org/r/216002/#review223846

Comment 31

[Markus Stange [:mstange]]

Comment on attachment 8945919 [details]
Bug 1348959 - Make ProfileBuffer fields uint32_t.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215998/diff/3-4/

Comment 32

[Markus Stange [:mstange]]

Comment on attachment 8945920 [details]
Bug 1348959 - Make profiler_get_buffer_info() return information in a struct instead of using outparams.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216000/diff/3-4/

Comment 33

[Markus Stange [:mstange]]

Comment on attachment 8945921 [details]
Bug 1348959 - Remove wraparound indexing in ProfileBuffer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216002/diff/4-5/

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0f792f685c04
Make ProfileBuffer fields uint32_t. r=mystor,njn
https://hg.mozilla.org/integration/autoland/rev/5ca04ee79f78
Make profiler_get_buffer_info() return information in a struct instead of using outparams. r=mystor,njn
https://hg.mozilla.org/integration/autoland/rev/1406e2ac322a
Remove wraparound indexing in ProfileBuffer. r=djvj,mystor,njn

Comment 35

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/0f792f685c04
https://hg.mozilla.org/mozilla-central/rev/5ca04ee79f78
https://hg.mozilla.org/mozilla-central/rev/1406e2ac322a