Closed Bug 1208747 Opened 9 years ago Closed 9 years ago

Try to keep as much as possible of the Stopwatch out of js/

Categories

(Toolkit :: Performance Monitoring, defect)

Product:
Toolkit
Component:
Performance Monitoring
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla44
Tracking Flags:
Tracking Status
firefox44 --- fixed
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

Attachments

(5 files, 2 obsolete files)

MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level + XPConnect-level);r=froydnj
40 bytes, text/x-review-board-request
froydnj
: review+
Details
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JSAPI-level);r=jandem
40 bytes, text/x-review-board-request
jandem
: review+
Details
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JS-level);r=felipe
40 bytes, text/x-review-board-request
Felipe
: review+
Details
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (documentation);r=froydnj
40 bytes, text/x-review-board-request
froydnj
: review+
Details
bug1208747-includes.patch
1.53 KB, patch
jandem
: review+
Details | Diff | Splinter Review 
      No description provided.
As discussed by jandem in bug 1188248.
Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JSAPI-level);r?jandem

The Performance Monitoring feature of SpiderMonkey is growing quite large and possibly too Firefox-specific. As suggested by jandem, we can actually remove large chunks of this code from js/ and move
them to toolkit/, hence reducing the SpiderMonkey TCB and making the feature more generic. This patch deals with with the JSAPI-based code.

Main changes:

1/ Most of the code disappears from js/.

2/ `js::PerformanceGroup` is now defined in jsapi.h

3/ `js::PerformanceGroup` is now allocated and deallocated by the embedding.
Attachment #8666361 - Flags: review?(jdemooij)
Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPConnect-level);r?bholley
Attachment #8666362 - Flags: review?(bobbyholley)
Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level);r?froydnj

This patch reimplements at XPCOM-level most of the performance-monirogin work that was done in SpiderMonkey.

A few changes:

1/ nsIPerformanceStats loses the `parentId`.
It was annoying to maintain, and quite useless so far. Future patches may resurrect it in a different form, once we have decided if/how we wish to use it.

2/ nsIPerformanceStats loses the `title` field.
With the new implementation, maintaining this field would have been very costly (or, with planned evolutions of this module, impossible). It can, however, be done easily in JavaScript, since we still capture the `windowId`.

3/ We now capture data on compartments eagerly.
The previous implementation captured data on compartments each time we performed a snapshot. This will not be possible once we add the notifications API (bug 1186491), plus this would be much more complicated to do or much more costly now that we are doing our work in XPCOM-land. Instead, we now capture data when compartments are visited for the first time after initialization of the performance service. This is slightly more costly during startup and slightly more memory-consuming. However, this is also slightly faster during runtime, the code is cleaner and more importantly, this permits the notification-based API, and this opens the way to having more types of groups, e.g. grouping compartments by domain.

4/ A compartment can now belong to both an add-on and a window.
If a compartment was involved in executing code on behalf of an add-on in a window, the previous implementation decided arbitrarily that the code belonged to either the add-on or the window. This implementation now associates the compartment to both. This is one of the reasons for which `parentId` does not make sense anymore, by the way.
Comment on attachment 8666364 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JS-level);r=felipe

https://reviewboard.mozilla.org/r/20537/#review18419

tip: for some reason, MozReview didn't pick me up for reviewing this, and it looks like it missed for part 3 of your patch to Nathan too.
Comment on attachment 8666361 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level + XPConnect-level);r=froydnj

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JSAPI-level);r?jandem

The Performance Monitoring feature of SpiderMonkey is growing quite large and possibly too Firefox-specific. As suggested by jandem, we can actually remove large chunks of this code from js/ and move
them to toolkit/, hence reducing the SpiderMonkey TCB and making the feature more generic. This patch deals with with the JSAPI-based code.

Main changes:

1/ Most of the code disappears from js/.

2/ `js::PerformanceGroup` is now defined in jsapi.h

3/ `js::PerformanceGroup` is now allocated and deallocated by the embedding.
Comment on attachment 8666362 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JSAPI-level);r=jandem

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPConnect-level);r?bholley
Comment on attachment 8666363 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level);r?froydnj

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level);r?froydnj

This patch reimplements at XPCOM-level most of the performance-monirogin work that was done in SpiderMonkey.

A few changes:

1/ nsIPerformanceStats loses the `parentId`.
It was annoying to maintain, and quite useless so far. Future patches may resurrect it in a different form, once we have decided if/how we wish to use it.

2/ nsIPerformanceStats loses the `title` field.
With the new implementation, maintaining this field would have been very costly (or, with planned evolutions of this module, impossible). It can, however, be done easily in JavaScript, since we still capture the `windowId`.

3/ We now capture data on compartments eagerly.
The previous implementation captured data on compartments each time we performed a snapshot. This will not be possible once we add the notifications API (bug 1186491), plus this would be much more complicated to do or much more costly now that we are doing our work in XPCOM-land. Instead, we now capture data when compartments are visited for the first time after initialization of the performance service. This is slightly more costly during startup and slightly more memory-consuming. However, this is also slightly faster during runtime, the code is cleaner and more importantly, this permits the notification-based API, and this opens the way to having more types of groups, e.g. grouping compartments by domain.

4/ A compartment can now belong to both an add-on and a window.
If a compartment was involved in executing code on behalf of an add-on in a window, the previous implementation decided arbitrarily that the code belonged to either the add-on or the window. This implementation now associates the compartment to both. This is one of the reasons for which `parentId` does not make sense anymore, by the way.
Attachment #8666363 - Flags: review?(nfroyd)
Comment on attachment 8666364 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JS-level);r=felipe

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JS-level);r?felipe
Comment on attachment 8666362 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JSAPI-level);r=jandem

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPConnect-level);r?bholley
Comment on attachment 8666363 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level);r?froydnj

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level);r?froydnj

This patch reimplements at XPCOM-level most of the performance-monirogin work that was done in SpiderMonkey.

A few changes:

1/ nsIPerformanceStats loses the `parentId`.
It was annoying to maintain, and quite useless so far. Future patches may resurrect it in a different form, once we have decided if/how we wish to use it.

2/ nsIPerformanceStats loses the `title` field.
With the new implementation, maintaining this field would have been very costly (or, with planned evolutions of this module, impossible). It can, however, be done easily in JavaScript, since we still capture the `windowId`.

3/ We now capture data on compartments eagerly.
The previous implementation captured data on compartments each time we performed a snapshot. This will not be possible once we add the notifications API (bug 1186491), plus this would be much more complicated to do or much more costly now that we are doing our work in XPCOM-land. Instead, we now capture data when compartments are visited for the first time after initialization of the performance service. This is slightly more costly during startup and slightly more memory-consuming. However, this is also slightly faster during runtime, the code is cleaner and more importantly, this permits the notification-based API, and this opens the way to having more types of groups, e.g. grouping compartments by domain.

4/ A compartment can now belong to both an add-on and a window.
If a compartment was involved in executing code on behalf of an add-on in a window, the previous implementation decided arbitrarily that the code belonged to either the add-on or the window. This implementation now associates the compartment to both. This is one of the reasons for which `parentId` does not make sense anymore, by the way.
Comment on attachment 8666364 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JS-level);r=felipe

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JS-level);r?felipe
Comment on attachment 8666362 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JSAPI-level);r=jandem

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPConnect-level);r?bholley
Comment on attachment 8666363 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level);r?froydnj

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level);r?froydnj

This patch reimplements at XPCOM-level most of the performance-monirogin work that was done in SpiderMonkey.

A few changes:

1/ nsIPerformanceStats loses the `parentId`.
It was annoying to maintain, and quite useless so far. Future patches may resurrect it in a different form, once we have decided if/how we wish to use it.

2/ nsIPerformanceStats loses the `title` field.
With the new implementation, maintaining this field would have been very costly (or, with planned evolutions of this module, impossible). It can, however, be done easily in JavaScript, since we still capture the `windowId`.

3/ We now capture data on compartments eagerly.
The previous implementation captured data on compartments each time we performed a snapshot. This will not be possible once we add the notifications API (bug 1186491), plus this would be much more complicated to do or much more costly now that we are doing our work in XPCOM-land. Instead, we now capture data when compartments are visited for the first time after initialization of the performance service. This is slightly more costly during startup and slightly more memory-consuming. However, this is also slightly faster during runtime, the code is cleaner and more importantly, this permits the notification-based API, and this opens the way to having more types of groups, e.g. grouping compartments by domain.

4/ A compartment can now belong to both an add-on and a window.
If a compartment was involved in executing code on behalf of an add-on in a window, the previous implementation decided arbitrarily that the code belonged to either the add-on or the window. This implementation now associates the compartment to both. This is one of the reasons for which `parentId` does not make sense anymore, by the way.
Comment on attachment 8666364 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JS-level);r=felipe

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JS-level);r?felipe
I don't think I'm going to be able to get to this until Wednesday.

Is there a better way to split this patch up?  Looking at the diff, it looks like a mess.  And the diff summary makes it look like it's just moving hunks of code around in nsPerformanceStats.cpp?
Flags: needinfo?(dteller)
Thanks for picking up this review. Mossop, who reviewed previous versions of the component, declared himself too out of touch with C++ XPCOM to continue reviewing the code now that it actually does something.

Given that this patch essentially rewrites nsPerformanceStats.*, I believe that it would be easier to look at the entire file rather than the diff.
Flags: needinfo?(dteller)
Comment on attachment 8666362 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JSAPI-level);r=jandem

This diff doesn't make sense to me in isolation - it seems to be adding various dispose-related things on the SM side, and removing PerformanceGroup stuff on the XPConnect side.

Assuming that this bug is just moving code around, it probably makes sense to have one person review it all. I'm happy for someone else to review any changes that end up being in XPConnect.
Attachment #8666362 - Flags: review?(bobbyholley)
Ah, I just realized that I had squashed the js/src and the js/xpconnect patch incorrectly. Sorry about that.
Assignee: nobody → dteller
Attachment #8666362 - Attachment description: MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPConnect-level);r?bholley → MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JSAPI-level);r?jandem
Attachment #8666362 - Flags: review?(jdemooij)
Comment on attachment 8666362 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JSAPI-level);r=jandem

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JSAPI-level);r?jandem
Comment on attachment 8666361 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level + XPConnect-level);r=froydnj

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPConnect-level);r?bholley
Attachment #8666361 - Attachment description: MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JSAPI-level);r?jandem → MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPConnect-level);r?bholley
Attachment #8666361 - Flags: review?(jdemooij) → review?(bobbyholley)
Comment on attachment 8666363 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level);r?froydnj

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level);r?froydnj

This patch reimplements at XPCOM-level most of the performance-monirogin work that was done in SpiderMonkey.

A few changes:

1/ nsIPerformanceStats loses the `parentId`.
It was annoying to maintain, and quite useless so far. Future patches may resurrect it in a different form, once we have decided if/how we wish to use it.

2/ nsIPerformanceStats loses the `title` field.
With the new implementation, maintaining this field would have been very costly (or, with planned evolutions of this module, impossible). It can, however, be done easily in JavaScript, since we still capture the `windowId`.

3/ We now capture data on compartments eagerly.
The previous implementation captured data on compartments each time we performed a snapshot. This will not be possible once we add the notifications API (bug 1186491), plus this would be much more complicated to do or much more costly now that we are doing our work in XPCOM-land. Instead, we now capture data when compartments are visited for the first time after initialization of the performance service. This is slightly more costly during startup and slightly more memory-consuming. However, this is also slightly faster during runtime, the code is cleaner and more importantly, this permits the notification-based API, and this opens the way to having more types of groups, e.g. grouping compartments by domain.

4/ A compartment can now belong to both an add-on and a window.
If a compartment was involved in executing code on behalf of an add-on in a window, the previous implementation decided arbitrarily that the code belonged to either the add-on or the window. This implementation now associates the compartment to both. This is one of the reasons for which `parentId` does not make sense anymore, by the way.
Comment on attachment 8666364 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JS-level);r=felipe

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JS-level);r?felipe
Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (documentation);r?froydnj
Attachment #8668882 - Flags: review?(nfroyd)
Nathan, since you have the most complicated task in this review, I figured some documentation could help.
Attachment #8666363 - Flags: review?(nfroyd)
Comment on attachment 8666363 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level);r?froydnj

https://reviewboard.mozilla.org/r/20535/#review18975

This is mostly OK, lots of little details.  Fortunately, MozReview makes it easy to provide interdiffs!

::: toolkit/components/perfmonitoring/nsPerformanceStats.h:115
(Diff revision 5)
> +    class nsPerformanceGroup* mGroup;

Just forward declare this class somewhere so you don't have to clutter things up with |class| keywords.

::: toolkit/components/perfmonitoring/nsPerformanceStats.h:124
(Diff revision 5)
> +  struct WindowIdToGroup: public nsUint32HashKey {
> +    WindowIdToGroup(const uint32_t* key)
> +      : nsUint32HashKey(key)
> +      , mGroup(nullptr)
> +    {}

This should be inheriting from nsUint64HashKey, with concomitant changes all around.

::: toolkit/components/perfmonitoring/nsPerformanceStats.h:129
(Diff revision 5)
> +    class nsPerformanceGroup* mGroup;

Since nsPerformanceGroup is refcounted, who is holding the reference(s) that ensure these pointers are valid?

Alternatively, you could use nsRefPtrHashtable here...

::: toolkit/components/perfmonitoring/nsPerformanceStats.h:293
(Diff revision 5)
> +  uint64_t durations[10];

'm' prefixes on all the members in this class, please.

::: toolkit/components/perfmonitoring/nsPerformanceStats.h:326
(Diff revision 5)
> +                    const uint32_t aWindowId,

Window IDs are 64-bit integers prior to getting passed into this class, but 32-bit integers once they get inside this class.  We shouldn't truncate like that.

::: toolkit/components/perfmonitoring/nsPerformanceStats.h:377
(Diff revision 5)
> +   * @param isSystem `true` if the code of the group is executed with
> +   *   system credentials, `false` otherwise.
> +   * @param isShared `true` if this group represents either all compartments
> +   *   executed in a window or all compartments executed in an addon. In
> +   *   either case, this group is in charge of removing itself from
> +   *   mWindowIdToGroup/mAddonIdToGroup upon destruction.

I strongly encourage you to use named enums for these, rather than bare boolean parameters.

::: toolkit/components/perfmonitoring/nsPerformanceStats.h:397
(Diff revision 5)
> +  static inline nsPerformanceGroup* Get(js::PerformanceGroup* self) {
> +    return reinterpret_cast<nsPerformanceGroup*>(self);
> +  }
> +  static inline const nsPerformanceGroup* Get(const js::PerformanceGroup* self) {
> +    return reinterpret_cast<const nsPerformanceGroup*>(self);

I believe static_cast can be used here, and is preferable to reinterpret_cast.

::: toolkit/components/perfmonitoring/nsPerformanceStats.cpp:84
(Diff revision 5)
> +GetName(JSContext* cx, JS::Handle<JSObject*> global, nsAString& name) {

Please call this NameForContext.

::: toolkit/components/perfmonitoring/nsPerformanceStats.cpp:85
(Diff revision 5)
> +  nsAutoCString cname;
> +  do {
> +    // Attempt to use the URL as name.
> +    nsCOMPtr<nsIPrincipal> principal = nsContentUtils::ObjectPrincipal(global);

I wonder if this wouldn't be better as a function:

bool NameForGlobal(JS::Handle<JSObject*> global, nsAString& name);

Then one doesn't need to use the clever do-while-break loop form.

::: toolkit/components/perfmonitoring/nsPerformanceStats.cpp:115
(Diff revision 5)
> +GetGroupId(const JSRuntime* rt, uint64_t uid, uint64_t processId, nsAString& groupId) {

Please call this GenerateUniqueGroupId.

::: toolkit/components/perfmonitoring/nsPerformanceStats.cpp:116
(Diff revision 5)
> +  uint64_t runtimeId = reinterpret_cast<uintptr_t>(rt);

I guess this is original code, but please make this static_cast.

::: toolkit/components/perfmonitoring/nsPerformanceStats.cpp:145
(Diff revision 5)
> +PerformanceData::PerformanceData(const PerformanceData& from)

I think you could just |= default| the copy constructor and assignment and they would work just fine, rather than writing ~20 lines of code for them.

::: toolkit/components/perfmonitoring/nsPerformanceStats.cpp:172
(Diff revision 5)
> +nsPerformanceItem::GetName() const {

I think for all of these accessors, we should drop the Get prefix; we're not in XPCOM-land, and we don't need to follow the Gecko conventions of prefixing with Get if things return null.

::: toolkit/components/perfmonitoring/nsPerformanceStats.cpp:498
(Diff revision 5)
> -
> -  return true;
> +  if (!js::SetStopwatchCommitCallback(mRuntime, StopwatchCommitCallback, this)) {
> +    return NS_ERROR_UNEXPECTED;

It seems a little odd that this API can leave things in a half-finished state: setting the start callback can succeed, but setting the commit callback could fail?

Or, a little further down, we could successfully set our stopwatch callbacks, but fail to set the performance groups callback?

::: toolkit/components/perfmonitoring/nsPerformanceStats.cpp:673
(Diff revision 5)
> +    return true;

Since we return true on all paths from this function, I don't know that having it return bool is really necessary?  Having it return a JSGroupVector would likely make the code cleaner and eliminate the need to clear the outparam (unnecessarily?).

::: toolkit/components/perfmonitoring/nsPerformanceStats.cpp:679
(Diff revision 5)
> +  nsString name;

nsAutoString, please.

::: toolkit/components/perfmonitoring/nsPerformanceStats.cpp:691
(Diff revision 5)
> +      nsString addonName = name;

nsAutoString, please.

::: toolkit/components/perfmonitoring/nsPerformanceStats.cpp:708
(Diff revision 5)
> +    windowId = ptop->WindowID();

WindowID returns a 64-bit integer.

::: toolkit/components/perfmonitoring/nsPerformanceStats.cpp:711
(Diff revision 5)
> +      nsString windowName = name;

nsAutoString, please.

::: toolkit/components/perfmonitoring/nsPerformanceStats.cpp:839
(Diff revision 5)
> +  for (i = 0, duration = 1000;

Since you already initialized duration when you declared it, you can remove this duplicate initialization, and then you can declare the loop index variable in the loop.

::: toolkit/components/perfmonitoring/nsPerformanceStats.cpp:842
(Diff revision 5)
> +    group->data.durations[i]++;

Mightn't it be better to make the durations histogram here non-cumulative, and then extract the cumulative histogram later if we need it, leaving the original data intact for other analysis?

::: toolkit/components/perfmonitoring/nsPerformanceStats.cpp:941
(Diff revision 5)
> +  nsString groupId;

nsAutoString, please.
Comment on attachment 8668882 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (documentation);r=froydnj

https://reviewboard.mozilla.org/r/21069/#review18979

Thank you for writing this up!  I think some of this would be better placed in IDL files, but having everything in one places is helpful.  Can you format this and tie it into the build system so that it appears as "official" Gecko documentation on readthedocs or something?
Attachment #8668882 - Flags: review?(nfroyd) → review+
Comment on attachment 8666361 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level + XPConnect-level);r=froydnj

As noted in comment 19, I don't think it makes sense to have one person review the removal of some machinery and another person review its addition. Feel free to be less strict about module boundaries, and include this in whichever patch contains the moved functionality.
Attachment #8666361 - Flags: review?(bobbyholley)
https://reviewboard.mozilla.org/r/20535/#review18975

> Since nsPerformanceGroup is refcounted, who is holding the reference(s) that ensure these pointers are valid?
> 
> Alternatively, you could use nsRefPtrHashtable here...

I didn't do this because that would create interesting cycles going through SpiderMonkey-managed memory.

A `nsPerformanceGroup` is typically kept alive (as a `js::PerformanceGroup`) by the `JSCompartment` to which it is associated. It may also temporarily be kept alive by the JS stack, in particular in case of nested event loops. The former is probably sufficient, but I went with refcounting as there was no strong guarantee that the gc would not garbage-collect a compartment while we were unrolling the stack.

Added a comment.

> I strongly encourage you to use named enums for these, rather than bare boolean parameters.

It doesn't really make sense for `isSystem`, but it certainly does for `isShared`.

> Please call this NameForContext.

I want for `GetCompartmentName`. It's actually not for the context itself (which is shared by all compartments).

> I wonder if this wouldn't be better as a function:
> 
> bool NameForGlobal(JS::Handle<JSObject*> global, nsAString& name);
> 
> Then one doesn't need to use the clever do-while-break loop form.

I went for `GetURLForGlobal`.

> I guess this is original code, but please make this static_cast.

In my experience, casting a pointer to a `uintptr_t` needs a `reinterpret_cast`.

> It seems a little odd that this API can leave things in a half-finished state: setting the start callback can succeed, but setting the commit callback could fail?
> 
> Or, a little further down, we could successfully set our stopwatch callbacks, but fail to set the performance groups callback?

I added some cleanup code in case initialization fails.

> Since we return true on all paths from this function, I don't know that having it return bool is really necessary?  Having it return a JSGroupVector would likely make the code cleaner and eliminate the need to clear the outparam (unnecessarily?).

Since this is a callback for JS, I'm trying to follow the JS conventions that has functions return `false` in case of error. The JS code knows how to handle an error here. If this saves me from having to refactor JSAPI code later to add error-handling, I'm rather satisfied with this return value.

Yes, clearing the outparam is probably overkill.

> nsAutoString, please.

What's the rationale? Since we pass it to a constructor that is going to store it for a long time, this means copying the data from stack/heap to a `nsString`. I seem to remember that assigning it to a `nsString` will avoid the copy. Same question for the other uses of `nsAutoString` you suggest below.

> Mightn't it be better to make the durations histogram here non-cumulative, and then extract the cumulative histogram later if we need it, leaving the original data intact for other analysis?

I don't follow what you mean here.
At bholley's request, I'm merging the XPConnect patch and the XPCOM patch. Don't worry, froydnj, the former is trivial :)
Comment on attachment 8666361 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level + XPConnect-level);r=froydnj

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level + XPConnect-level);r?froydnj

This patch reimplements at XPCOM-level most of the performance-monitoring. work that was done in SpiderMonkey.

A few changes:

1/ nsIPerformanceStats loses the `parentId`.
It was annoying to maintain, and quite useless so far. Future patches may resurrect it in a different form, once we have decided if/how we wish to use it.

2/ nsIPerformanceStats loses the `title` field.
With the new implementation, maintaining this field would have been very costly (or, with planned evolutions of this module, impossible). It can, however, be done easily in JavaScript, since we still capture the `windowId`.

3/ We now capture data on compartments eagerly.
The previous implementation captured data on compartments each time we performed a snapshot. This will not be possible once we add the notifications API (bug 1186491), plus this would be much more complicated to do or much more costly now that we are doing our work in XPCOM-land. Instead, we now capture data when compartments are visited for the first time after initialization of the performance service. This is slightly more costly during startup and slightly more memory-consuming. However, this is also slightly faster during runtime, the code is cleaner and more importantly, this permits the notification-based API, and this opens the way to having more types of groups, e.g. grouping compartments by domain.

4/ A compartment can now belong to both an add-on and a window.
If a compartment was involved in executing code on behalf of an add-on in a window, the previous implementation decided arbitrarily that the code belonged to either the add-on or the window. This implementation now associates the compartment to both. This is one of the reasons for which `parentId` does not make sense anymore, by the way.
Attachment #8666361 - Attachment description: MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPConnect-level);r?bholley → MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level + XPConnect-level);r?froydnj
Attachment #8666361 - Flags: review?(nfroyd)
Comment on attachment 8666364 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JS-level);r=felipe

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JS-level);r?felipe
Comment on attachment 8668882 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (documentation);r=froydnj

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (documentation);r?froydnj
Attachment #8666363 - Attachment is obsolete: true
(apparently, forgot to publish on MozReview)
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #30)
> > Since nsPerformanceGroup is refcounted, who is holding the reference(s) that ensure these pointers are valid?
> > 
> > Alternatively, you could use nsRefPtrHashtable here...
> 
> I didn't do this because that would create interesting cycles going through
> SpiderMonkey-managed memory.
> 
> Added a comment.

Excellent, thank you.

> > Please call this NameForContext.
> 
> I want for `GetCompartmentName`. It's actually not for the context itself
> (which is shared by all compartments).
> 
> > I wonder if this wouldn't be better as a function:
> > 
> > bool NameForGlobal(JS::Handle<JSObject*> global, nsAString& name);
> > 
> > Then one doesn't need to use the clever do-while-break loop form.
> 
> I went for `GetURLForGlobal`.

I was trying to apply the not-remembered-very-often Gecko rule (including by yours truly as a reviewer) that Get-prefixing something indicates that it can return nullptr.  Would you mind dropping the 'Get' prefix?

> > I guess this is original code, but please make this static_cast.
> 
> In my experience, casting a pointer to a `uintptr_t` needs a
> `reinterpret_cast`.

Pffft on your experience. ;)  That misreading of the code is entirely on me.

> > It seems a little odd that this API can leave things in a half-finished state: setting the start callback can succeed, but setting the commit callback could fail?
> > 
> > Or, a little further down, we could successfully set our stopwatch callbacks, but fail to set the performance groups callback?
> 
> I added some cleanup code in case initialization fails.

Thanks.  (I'm not sure how likely this is to fail, but...)

> > Since we return true on all paths from this function, I don't know that having it return bool is really necessary?  Having it return a JSGroupVector would likely make the code cleaner and eliminate the need to clear the outparam (unnecessarily?).
> 
> Since this is a callback for JS, I'm trying to follow the JS conventions
> that has functions return `false` in case of error. The JS code knows how to
> handle an error here. If this saves me from having to refactor JSAPI code
> later to add error-handling, I'm rather satisfied with this return value.

That's fair.

> > nsAutoString, please.
> 
> What's the rationale? Since we pass it to a constructor that is going to
> store it for a long time, this means copying the data from stack/heap to a
> `nsString`. I seem to remember that assigning it to a `nsString` will avoid
> the copy. Same question for the other uses of `nsAutoString` you suggest
> below.

Want to become an XPCOM string peer? >:D

I think you're right.  TIL.

> > Mightn't it be better to make the durations histogram here non-cumulative, and then extract the cumulative histogram later if we need it, leaving the original data intact for other analysis?
> 
> I don't follow what you mean here.

I guess this doesn't matter too much for this case, since it looks like the cumulative histogram is what's used externally regardless.  What I was trying to suggest is given a stream of timings:

t1, t2, ..., tn

We're currently storing them in something that looks like:

0: +++++++++++++++++++++++++++
1: ++++++++++++++++++++++++++
2: ++++++++++++++++++++++++
3: +++++++++++++++++++
...
9: +++

I suggest storing them like:

0: ++++++++++++++
1: +++++++++++
2: ++++++++
3: ++++
4: +++++
....
9: +++

Because you can derive the former from the latter, trivially...but writing this, I guess you can go the other way, too.  I think the latter is easier to understand, though...but maybe not in the context of whatever performance analysis/presentation you want to do...or maybe because I have notn had enough experience reading these sorts of histograms.
Attachment #8666361 - Flags: review?(nfroyd) → review+
Comment on attachment 8666361 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level + XPConnect-level);r=froydnj

https://reviewboard.mozilla.org/r/20531/#review19379

Thanks for addressing issues and pushing back on my poor review.  A few comments below.

::: toolkit/components/perfmonitoring/nsPerformanceStats.h:350
(Diff revision 4)
> +  bool IsAddon() const;
> +  bool IsWindow() const;
> +  bool IsSystem() const;

Is it just me, or are these not defined anywhere?  I can see them called, but I don't see them grepping around in the patch (even in the complete diff).  I think they should be defined--based on my reading of the patch--something like:

bool IsAddon() const { return !mAddonId.IsEmpty(); }
bool IsWindow() const { return mWindowId != 0; }
bool IsSystem() const { return mIsSystem; }

If so, I'm OK with you just adding them.  Otherwise, I'd really like to know how they are defined. :)

I do like their use in MOZ_ASSERTs, so ++ for that.

::: toolkit/components/perfmonitoring/nsPerformanceStats.h:446
(Diff revision 4)
> +  Scope GetScope() const;

Same comment as comment 36 re: "Get" prefixes.

::: toolkit/components/perfmonitoring/nsPerformanceStats.cpp:80
(Diff revision 4)
> +bool
> +GetURLForGlobal(JSContext* cx, JS::Handle<JSObject*> global, nsAString& url) {

I guess this can fail, so perhaps the "Get" prefix is indeed appropriate here?
I initially adopted this presentation of histograms for two reasons:
- it makes it easy to find out if the code caused at least n milliseconds of jank;
– I didn't want to turn level 9 in a special case to deal with stuff that caused more than 2^9 milliseconds of jank.

I can, of course, change this behavior. I'll just have to be careful with the JS code that manipulates the data.
https://reviewboard.mozilla.org/r/20531/#review19379

> Is it just me, or are these not defined anywhere?  I can see them called, but I don't see them grepping around in the patch (even in the complete diff).  I think they should be defined--based on my reading of the patch--something like:
> 
> bool IsAddon() const { return !mAddonId.IsEmpty(); }
> bool IsWindow() const { return mWindowId != 0; }
> bool IsSystem() const { return mIsSystem; }
> 
> If so, I'm OK with you just adding them.  Otherwise, I'd really like to know how they are defined. :)
> 
> I do like their use in MOZ_ASSERTs, so ++ for that.

Er...
I'm sure that code must be somewhere. Really. Perhaps behind the curtain, or in my other pocket?

> I guess this can fail, so perhaps the "Get" prefix is indeed appropriate here?

Ah, well, I followed your previous remark and renamed it `URLForGlobal`.
If you're ok with this, I'd like to finish this bug without changing the definition of histograms, as this will likely impact the JS code. We can discuss that in a followup bug.
Flags: needinfo?(nfroyd)
Comment on attachment 8666361 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level + XPConnect-level);r=froydnj

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level + XPConnect-level);r?froydnj

This patch reimplements at XPCOM-level most of the performance-monitoring. work that was done in SpiderMonkey.

A few changes:

1/ nsIPerformanceStats loses the `parentId`.
It was annoying to maintain, and quite useless so far. Future patches may resurrect it in a different form, once we have decided if/how we wish to use it.

2/ nsIPerformanceStats loses the `title` field.
With the new implementation, maintaining this field would have been very costly (or, with planned evolutions of this module, impossible). It can, however, be done easily in JavaScript, since we still capture the `windowId`.

3/ We now capture data on compartments eagerly.
The previous implementation captured data on compartments each time we performed a snapshot. This will not be possible once we add the notifications API (bug 1186491), plus this would be much more complicated to do or much more costly now that we are doing our work in XPCOM-land. Instead, we now capture data when compartments are visited for the first time after initialization of the performance service. This is slightly more costly during startup and slightly more memory-consuming. However, this is also slightly faster during runtime, the code is cleaner and more importantly, this permits the notification-based API, and this opens the way to having more types of groups, e.g. grouping compartments by domain.

4/ A compartment can now belong to both an add-on and a window.
If a compartment was involved in executing code on behalf of an add-on in a window, the previous implementation decided arbitrarily that the code belonged to either the add-on or the window. This implementation now associates the compartment to both. This is one of the reasons for which `parentId` does not make sense anymore, by the way.
Comment on attachment 8666364 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JS-level);r=felipe

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JS-level);r?felipe
Comment on attachment 8668882 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (documentation);r=froydnj

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (documentation);r?froydnj
Comment on attachment 8666362 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JSAPI-level);r=jandem

https://reviewboard.mozilla.org/r/20533/#review19455

Thanks for doing this! r=me with comments below addressed.

::: js/src/jsapi.h:5487
(Diff revision 5)
> +protected:

Indent with 2 spaces.

::: js/src/jsapi.h:5492
(Diff revision 5)
>  private:

and here.

::: js/src/jsapi.h:5523
(Diff revision 5)
> -  private:
> +public:

And here.

::: js/src/vm/Stopwatch.cpp:10
(Diff revision 5)
> +PerformanceMonitoring::addRecentGroup(PerformanceGroup* group) {

Nit: give the { its own line, also for all the methods/functions below.

::: js/src/vm/Stopwatch.cpp:15
(Diff revision 5)
> +  recentGroups_.append(group);

This should propagate failure:

  return recentGroups_.append(group);

And make sure the callers deal with that.

::: js/src/vm/Stopwatch.cpp:130
(Diff revision 5)
> +      stopwatchCommitCallback(iteration_, recentGroups, stopwatchCommitClosure);

Indent with 2 more spaces. Also, isn't the callback fallible? Since you're passing it a Vector, I assume it's going to append to it.

::: js/src/vm/Stopwatch.cpp:142
(Diff revision 5)
> +    c->performanceMonitoring.unlink();

Indent this method with 4 instead of 2 spaces.

::: js/src/vm/Stopwatch.cpp:158
(Diff revision 5)
> +  if (initialized_)

And this one.

::: js/src/vm/Stopwatch.cpp:193
(Diff revision 5)
> +      return;

2 more spaces so it's a multiple of 4, also the loops and ifs below.

::: js/src/vm/Stopwatch.cpp:231
(Diff revision 5)
> +      releaseGroup(*group);

2 more spaces.

::: js/src/vm/Stopwatch.cpp:239
(Diff revision 5)
> +      CPOWTimeStart_ = runtime->performanceMonitoring.totalCPOWTime;

2 more spaces.

::: js/src/vm/Stopwatch.cpp:244
(Diff revision 5)
> +      cyclesStart_ = this->getCycles();

and here
Attachment #8666362 - Flags: review?(jdemooij) → review+
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #40)
> If you're ok with this, I'd like to finish this bug without changing the
> definition of histograms, as this will likely impact the JS code. We can
> discuss that in a followup bug.

That's fine with me.
Flags: needinfo?(nfroyd)
Comment on attachment 8666362 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JSAPI-level);r=jandem

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JSAPI-level);r=jandem
Attachment #8666362 - Attachment description: MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JSAPI-level);r?jandem → MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JSAPI-level);r=jandem
Comment on attachment 8666361 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level + XPConnect-level);r=froydnj

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level + XPConnect-level);r=froydnj
Attachment #8666361 - Attachment description: MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level + XPConnect-level);r?froydnj → MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level + XPConnect-level);r=froydnj
Comment on attachment 8666364 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JS-level);r=felipe

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JS-level);r=felipe
Attachment #8666364 - Attachment description: MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JS-level);r?felipe → MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JS-level);r=felipe
Comment on attachment 8668882 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (documentation);r=froydnj

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (documentation);r=froydnj
Attachment #8668882 - Attachment description: MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (documentation);r?froydnj → MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (documentation);r=froydnj
Comment on attachment 8666361 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level + XPConnect-level);r=froydnj

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level + XPConnect-level);r=froydnj
Comment on attachment 8666364 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JS-level);r=felipe

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JS-level);r=felipe
Comment on attachment 8668882 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (documentation);r=froydnj

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (documentation);r=froydnj
Comment on attachment 8666362 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JSAPI-level);r=jandem

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JSAPI-level);r=jandem
Comment on attachment 8666361 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level + XPConnect-level);r=froydnj

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level + XPConnect-level);r=froydnj
Comment on attachment 8666364 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JS-level);r=felipe

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JS-level);r=felipe
Comment on attachment 8668882 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (documentation);r=froydnj

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (documentation);r=froydnj
Comment on attachment 8666362 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JSAPI-level);r=jandem

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JSAPI-level);r=jandem
Comment on attachment 8666361 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level + XPConnect-level);r=froydnj

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level + XPConnect-level);r=froydnj
Comment on attachment 8666364 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JS-level);r=felipe

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JS-level);r=felipe
Comment on attachment 8668882 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (documentation);r=froydnj

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (documentation);r=froydnj
Comment on attachment 8666362 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JSAPI-level);r=jandem

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JSAPI-level);r=jandem
Comment on attachment 8666361 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level + XPConnect-level);r=froydnj

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level + XPConnect-level);r=froydnj
Comment on attachment 8666364 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JS-level);r=felipe

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JS-level);r=felipe
Comment on attachment 8668882 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (documentation);r=froydnj

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (documentation);r=froydnj
Needs rebasing.
Keywords: checkin-needed
Comment on attachment 8666362 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JSAPI-level);r=jandem

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JSAPI-level);r=jandem
Comment on attachment 8666361 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level + XPConnect-level);r=froydnj

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level + XPConnect-level);r=froydnj
Comment on attachment 8666364 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JS-level);r=felipe

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JS-level);r=felipe
Comment on attachment 8668882 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (documentation);r=froydnj

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (documentation);r=froydnj
the 2nd patch here cause on commit:

remote: *************************** ERROR ***************************
remote: Push rejected because the following IDL interfaces were
remote: modified without changing the UUID:
remote:   - nsIPerformanceStats in changeset de8e470ef6c9
remote: 
remote: To update the UUID for all of the above interfaces and their
remote: descendants, run:
remote:   ./mach update-uuids nsIPerformanceStats
remote: 
remote: If you intentionally want to keep the current UUID, include
remote: 'IGNORE IDL' in the commit message.
remote: *************************************************************
Flags: needinfo?(dteller)
Comment on attachment 8666362 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JSAPI-level);r=jandem

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JSAPI-level);r=jandem
Comment on attachment 8666361 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level + XPConnect-level);r=froydnj

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level + XPConnect-level);r=froydnj
Comment on attachment 8666364 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JS-level);r=felipe

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JS-level);r=felipe
Comment on attachment 8668882 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (documentation);r=froydnj

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (documentation);r=froydnj
Updated uuids.
Flags: needinfo?(dteller)
Comment on attachment 8666362 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JSAPI-level);r=jandem

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JSAPI-level);r=jandem
Comment on attachment 8666361 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level + XPConnect-level);r=froydnj

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (XPCOM-level + XPConnect-level);r=froydnj
Comment on attachment 8666364 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JS-level);r=felipe

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (JS-level);r=felipe
Comment on attachment 8668882 [details]
MozReview Request: Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (documentation);r=froydnj

Bug 1208747 - Move most of Stopwatch-related code to XPCOM-land (documentation);r=froydnj
Fixed.
Flags: needinfo?(dteller)
What remains of this code in SpiderMonkey does not compile on my Windows 7 system with 32-bit Visual C++ Community 2013.  The type PROCESSOR_NUMBER is undefined.

David, are nonstandard headers required?
Flags: needinfo?(dteller)
@lth: it is defined here: https://msdn.microsoft.com/en-us/library/windows/desktop/dd405505%28v=vs.85%29.aspx

According to the documentation, it requires Windows 7, so I can't think of anything wrong with your config. The code that uses it is not too important, though, so I guess we could commit it out.
Flags: needinfo?(dteller)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch bug1208747-include-files.patch (obsolete) — — Splinter Review 
Adjustment: When the stopwatch code was moved from Interpreter.cpp, the code that includes the necessary Windows headers should also have been moved.

(Followup bug: Maybe remove the code from Interpreter.cpp, if it's no longer needed there.)
Attachment #8686571 - Flags: review?(jdemooij)
(In reply to Lars T Hansen [:lth] from comment #93)
> Created attachment 8686571 [details] [diff] [review]
> bug1208747-include-files.patch
> 
> Adjustment: When the stopwatch code was moved from Interpreter.cpp, the code
> that includes the necessary Windows headers should also have been moved.

(Was probably moved from Runtime.cpp, I may have misread the patch.  Not important.)
I'm probably running into this and a similar problem on arm64 because I have a patch queue that removes a cpp file, so I'm changing the unified builds.
This also fixes build errors on ARM64 (Simulator) that I did not address yesterday and that occur only on Try.
Attachment #8686571 - Attachment is obsolete: true
Attachment #8686571 - Flags: review?(jdemooij)
Attachment #8687107 - Flags: review?(jdemooij)
Blocks: 1176214
Comment on attachment 8687107 [details] [diff] [review]
bug1208747-includes.patch

Review of attachment 8687107 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Stopwatch.cpp
@@ +5,5 @@
>  #include "mozilla/unused.h"
>  
> +#if defined(XP_WIN)
> +#include <processthreadsapi.h>
> +#include <windows.h>

The code in Interpreter.cpp was the same, but this should be #include "jswin.h" probably. It has its own #ifdef XP_WIN, so we could also move it after the jscompartment.h include below.
Attachment #8687107 - Flags: review?(jdemooij) → review+
See Also: → 1224588
https://hg.mozilla.org/mozilla-central/rev/efa3552ee031
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Depends on: 1261702
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: