Closed Bug 1259403 Opened 7 years ago Closed 7 years ago

Tracelogger: small nits

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: h4writer, Assigned: h4writer)

Details

Attachments

(2 files, 1 obsolete file)

No description provided.
In the previous patch we moved the "internal logging" to also measure when we were enlarging the space.

For the logging we need 2 certainly available items, which we always had when doing we cleared the data. Since we are now also doing it when enlarging the space, this is not true anymore.

Therefore adjusted to check if 3 items are available. First item is the one we want to log. Plus 2 extra, one for the start and one for the stop message.
Assignee: nobody → hv1989
Attachment #8734331 - Flags: review?(bbouvier)
Attached patch Don't increase by 1 (obsolete) — Splinter Review
The code here was suboptimal. Between CONTINUOUSSPACE_LIMIT/2 and CONTINUOUSSPACE_LIMIT it would increase the size by 1 every time. That is offcourse very time consuming. Therefore immediately go to CONTINUOUSSPACE_LIMIT if we are between CONTINUOUSSPACE_LIMIT/2 and CONTINUOUSSPACE_LIMIT and need more space.
Attachment #8734332 - Flags: review?(bbouvier)
Comment on attachment 8734332 [details] [diff] [review]
Don't increase by 1

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

::: js/src/vm/TraceLoggingTypes.h
@@ +201,4 @@
>              nCapacity = size_ + count;
>  
> +        if (nCapacity * sizeof(T) > CONTINUOUSSPACE_LIMIT) {
> +            nCapacity = CONTINUOUSSPACE_LIMIT;

There is a discrepancy here: it seems that CONTINUOUSSPACE_LIMIT is used as both an absolute memory value (in the condition) and a number of elements in the buffer (in the assignment). So there is something wrong here. Should it be nCapacity = C_LIMIT / sizeof(T) ?

While you're here, can you make CONTINUOUSSPACE_LIMIT a static member of the class itself, instead?
Attachment #8734332 - Flags: review?(bbouvier)
Comment on attachment 8734331 [details] [diff] [review]
Make sure we have 3 items

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

Indeed.

::: js/src/vm/TraceLogging.cpp
@@ +551,5 @@
>      if (enabled == 0)
>          return;
>  
>      MOZ_ASSERT(traceLoggerState);
> +    if (!events.hasSpaceForAdd(3)) {

Can you add a one line comment above explaining the 3, please? Your explanation in the bug makes sense, but I'd rather not have future readers to have to go to this bug to understand this value.

@@ +586,5 @@
>  
>          // Log the time it took to flush the events as being from the
>          // Tracelogger.
>          if (graph.get()) {
> +            MOZ_ASSERT(events.capacity() - events.size() > 2);

There could be a function in ContinuousSpace returning capacity() - size(), or even doing this assertion (taking the number of available slots (here 2) as a param?). What do you think?
Attachment #8734331 - Flags: review?(bbouvier) → review+
Lost track of this bug. Good catch! Adjusted to always look at max_items instead to LIMIT and addressed raised questions.
Attachment #8734332 - Attachment is obsolete: true
Attachment #8738970 - Flags: review?(bbouvier)
Comment on attachment 8738970 [details] [diff] [review]
Don't increase by 1

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

Thanks!

::: js/src/vm/TraceLoggingTypes.h
@@ +161,5 @@
>          js_free(data_);
>          data_ = nullptr;
>      }
>  
> +    uint32_t max_items() {

Mind to make it a static method? Why not camelCase :-)? (MaxSize would be a great name, as it refers to the size, but that's personal taste)

@@ +207,4 @@
>              nCapacity = size_ + count;
>  
> +        if (nCapacity > max_items()) {
> +            nCapacity = max_items();

If you want to, we could hide some control flow here:

nCapacity = Min(Max(nCapacity, size_t + count), maxItems());
if (size_ + count > nCapacity)
  return false;

And that would be it.
Attachment #8738970 - Flags: review?(bbouvier) → review+
https://hg.mozilla.org/mozilla-central/rev/f12e5d7ee596
https://hg.mozilla.org/mozilla-central/rev/63f538a4fbce
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.