1259403 - Tracelogger: small nits

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Make sure we have 3 items

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.

Comment 2

[Hannes Verschore [:h4writer]]

Attachment: Don't increase by 1

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.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

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?

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

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?

Comment 5

[Hannes Verschore [:h4writer]]

Attachment: Don't increase by 1

Lost track of this bug. Good catch! Adjusted to always look at max_items instead to LIMIT and addressed raised questions.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

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.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f12e5d7ee596
https://hg.mozilla.org/integration/mozilla-inbound/rev/63f538a4fbce

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f12e5d7ee596
https://hg.mozilla.org/mozilla-central/rev/63f538a4fbce