719917 - Add some more network and plugin related SAMPLE_LABELs

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add some more network and plugin related SAMPLE_LABELs

This helps split Input::OnInputStreamReady

Comment 1

[Benoit Girard (:BenWa)]

Comment on attachment 590262 [details] [diff] [review]
Add some more network and plugin related SAMPLE_LABELs

r+ conditional on you reviewing https://bugzilla.mozilla.org/attachment.cgi?id=589984&action=edit.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Attachment: v2

Comment 3

[Jeff Muizelaar [:jrmuizel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ae43e3c400b

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/6ae43e3c400b

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Is SAMPLE_LABEL no-op in the release builds? or is it very very fast?
I'm asking because the patch added a SAMPLE_LABEL to a very hot code path.

I could wonder a bit why a but "Add some more network and plugin related SAMPLE_LABELs" added
something to EventListenerManager. 
Also, why was the macro added to nsEventListenerManager::HandleEventInternal and not to 
EventDispatcher or not to nsEventListenerManager::HandleEventSubType?

Comment 6

[Benoit Girard (:BenWa)]

(In reply to Olli Pettay [:smaug] from comment #5)
> Is SAMPLE_LABEL no-op in the release builds? or is it very very fast?
> I'm asking because the patch added a SAMPLE_LABEL to a very hot code path.
> 

It's not a no-op, but could easily be made into one should we choose. It should be really fast and cause no allocations.

> I could wonder a bit why a but "Add some more network and plugin related
> SAMPLE_LABELs" added
> something to EventListenerManager. 
> Also, why was the macro added to nsEventListenerManager::HandleEventInternal
> and not to 
> EventDispatcher or not to nsEventListenerManager::HandleEventSubType?

smaug: BenWa: so, still trying to understand why the macro was added to ELM
smaug: nsEventDispatcher::Dispatch would be a better place, I think

Jeff, we should reconsider the placement of this label.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

(In reply to Olli Pettay [:smaug] from comment #5)
> Is SAMPLE_LABEL no-op in the release builds? or is it very very fast?
> I'm asking because the patch added a SAMPLE_LABEL to a very hot code path.


It's quite cheap, but I wouldn't call it very very fast.

For the following code:

void food(void (*bar)(void)) {
    SAMPLE_LABEL("foo", "bar");
    bar();
}

we get:

0000000000000000        pushq   %rbp
0000000000000001        movq    %rsp,%rbp
0000000000000004        pushq   %rbx
0000000000000005        pushq   %rax
0000000000000006        movq    _stack_key_initialized(%rip),%rax
000000000000000d        testb   $0x01,(%rax)
0000000000000010        je      0x00000055
0000000000000012        movq    _pkey_stack(%rip),%rax
0000000000000019        movq    %rdi,%rbx
000000000000001c        movq    (%rax),%rdi
000000000000001f        callq   _pthread_getspecific
0000000000000024        movq    %rbx,%rdi
0000000000000027        movq    %rax,%rbx
000000000000002a        testq   %rbx,%rbx
000000000000002d        je      0x00000055
000000000000002f        cmpl    $0x000003ff,0x00004000(%rbx)
0000000000000039        jg      0x0000005d
000000000000003b        movslq  0x00004000(%rbx),%rax
0000000000000042        leaq    L_.str(%rip),%rcx
0000000000000049        movq    %rcx,(%rbx,%rax,8)
000000000000004d        incl    0x00004000(%rbx)
0000000000000053        jmp     0x00000063
0000000000000055        addq    $0x08,%rsp
0000000000000059        popq    %rbx
000000000000005a        popq    %rbp
000000000000005b        jmp     *%rdi
000000000000005d        incl    0x00004008(%rbx)
0000000000000063        call    *%rdi
0000000000000065        leaq    0x00004008(%rbx),%rax
000000000000006c        cmpl    food(void (*)()),(%rax)
000000000000006f        jle     0x00000075
0000000000000071        decl    (%rax)
0000000000000073        jmp     0x0000007b
0000000000000075        decl    0x00004000(%rbx)
000000000000007b        addq    $0x08,%rsp
000000000000007f        popq    %rbx
0000000000000080        popq    %rbp

I'd guess that the cost is no more than 100 cycles.

When I added this I wasn't aware that this code path was very hot, so I'd be happy to take this one out.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

So, any chance to get the ELM part fixed.