Closed Bug 1466783 Opened 6 years ago Closed 6 years ago

Avoid copying while passing the profiler data with IPC

Categories

(Core :: Gecko Profiler, enhancement)

Product:
Core
Component:
Gecko Profiler
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: canova, Assigned: canova)

References

Details

Attachments

(1 file)

Bug 1466783 - Avoid copying while passing the profiler data with IPC
59 bytes, text/x-review-board-request
mstange
: review+
Details 
See bug 1458246 comment 10 for explanation. We can avoid copying the data. Currently we have 2 places that we can fix. One is in ChunkedJSONWriteFunc::CopyData() and the other one is in CollectProfileOrEmptyString.
Comment on attachment 8987554 [details]
Bug 1466783 - Avoid copying while passing the profiler data with IPC

https://reviewboard.mozilla.org/r/252796/#review261602

Looks good.

I don't know how I feel about embedding knowledge about Shmem allocation so deeply inside the profiler code. It's probably not going to make it easier to move the profiler code out of libxul (bug 1437245), but on the other hand, I don't like any of the ideas that I had for making this API more generic.

Here's such an idea that uses a lambda to delegate the allocation of the buffer to the caller:

Shmem shmem;
profiler_get_profile_json_into_lazily_allocated_buffer([&](size_t allocationSize) {
  if (AllocShmem(allocationSize,
                 SharedMemory::TYPE_BASIC,
                 &shmem)) {
    return shmem.get<char>();
  }
  return nullptr;
});
// use shmem here

::: tools/profiler/core/platform.cpp:2663
(Diff revision 1)
>      delete samplerThread;
>    }
>  }
>  
> -UniquePtr<char[]>
> -profiler_get_profile(double aSinceTime, bool aIsShuttingDown)
> +static bool
> +profiler_fill_profiler_json_writer(SpliceableChunkedJSONWriter& aWriter,

This function is not exposed in GeckoProfiler.h, so let's use a more normal name for it. We only use the "profiler_*" pattern for public functions in GeckoProfiler.h.

For this function, I suggest WriteProfileToJSONWriter.

::: tools/profiler/core/platform.cpp:2692
(Diff revision 1)
> +profiler_get_profile(double aSinceTime, bool aIsShuttingDown)
> +{
> +  LOG("profiler_get_profile");
> +
> +  SpliceableChunkedJSONWriter b;
> +  if(!profiler_fill_profiler_json_writer(b, aSinceTime, aIsShuttingDown)) {

Missing a space after "if"

::: tools/profiler/core/platform.cpp:2705
(Diff revision 1)
> +                           double aSinceTime, bool aIsShuttingDown)
> +{
> +  LOG("profiler_get_profile_shmem");
> +
> +  SpliceableChunkedJSONWriter b;
> +  if(!profiler_fill_profiler_json_writer(b, aSinceTime, aIsShuttingDown)) {

Missing space here, too
Attachment #8987554 - Flags: review?(mstange) → review+
I procrastinated the Shmem allocation to ProfilerChild.cpp. So we were able to get rid of the Shmem and allocator from other files. There are a few changes, could you review again Markus?
Comment on attachment 8987554 [details]
Bug 1466783 - Avoid copying while passing the profiler data with IPC

https://reviewboard.mozilla.org/r/252796/#review261602

> Missing space here, too

Ugh, there is no C++ linter command in the mach, right? It would be great to catch these before sending the patch. I miss the `./mach test-tidy` on servo :)
Comment on attachment 8987554 [details]
Bug 1466783 - Avoid copying while passing the profiler data with IPC

https://reviewboard.mozilla.org/r/252796/#review261724

::: tools/profiler/gecko/ProfilerChild.cpp:105
(Diff revision 2)
> +        return shmem.get<char>();
> +      }
> +      return nullptr;
> +    },
> +    /* aSinceTime */ 0, /* aIsShuttingDown */ false);
> +  aResolve(std::move(shmem));

I had to move the shmem here since GatherProfileResolver only allows r-value reference as the argument. But I think this is the right approach. There are also some examples on the existing code: https://searchfox.org/mozilla-central/search?q=move%5C(.*shmem%5C)&case=false&regexp=true&path=
(Updated the patch after `./mach clang-format` enlightenment. Thanks `./mach help`!)
Comment on attachment 8987554 [details]
Bug 1466783 - Avoid copying while passing the profiler data with IPC

https://reviewboard.mozilla.org/r/252796/#review261798

::: tools/profiler/core/ProfileJSONWriter.h:44
(Diff revision 3)
>    void Write(const char* aStr) override;
> +  void CopyDataIntoLazilyAllocatedBuffer(
> +    std::function<char*(size_t)> aAllocator) const;
>    mozilla::UniquePtr<char[]> CopyData() const;
>    void Take(ChunkedJSONWriteFunc&& aOther);
> +  size_t GetTotalLength() const;

Since this method is now public, let's add a comment to it.

// Returns the byte length of the complete combined string, including the null terminator byte.

::: tools/profiler/core/platform.cpp:2699
(Diff revision 3)
>    return b.WriteFunc()->CopyData();
>  }
>  
>  void
> +profiler_get_profile_json_into_lazily_allocated_buffer(
> +  std::function<char*(size_t)> aAllocator,

Let's change this to a const reference of std::function, I think that saves a potential copy of the lambda and expresses that we don't intend to store the lambda anywhere. Here and in the CopyDataIntoLazilyAllocatedBuffer method

::: tools/profiler/gecko/ProfilerChild.cpp:106
(Diff revision 3)
> +      }
> +      return nullptr;
> +    },
> +    /* aSinceTime */ 0,
> +    /* aIsShuttingDown */ false);
> +  aResolve(std::move(shmem));

Yes, moving the shmem is the right thing to do.
Pushed by canaltinova@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/463f7fc23171
Avoid copying while passing the profiler data with IPC r=mstange
Oh, I forgot to add a reference after adding `const` to arguments. Will fix.
Flags: needinfo?(canaltinova)
Pushed by canaltinova@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/90f004457128
Avoid copying while passing the profiler data with IPC r=mstange
https://hg.mozilla.org/mozilla-central/rev/90f004457128
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: