Last Comment Bug 719917 - Add some more network and plugin related SAMPLE_LABELs
: Add some more network and plugin related SAMPLE_LABELs
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: General (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla12
Assigned To: Jeff Muizelaar [:jrmuizel]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-20 11:22 PST by Jeff Muizelaar [:jrmuizel]
Modified: 2012-02-11 05:29 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add some more network and plugin related SAMPLE_LABELs (5.64 KB, patch)
2012-01-20 11:22 PST, Jeff Muizelaar [:jrmuizel]
bgirard: review+
Details | Diff | Review
v2 (9.94 KB, patch)
2012-01-23 13:59 PST, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Review

Description Jeff Muizelaar [:jrmuizel] 2012-01-20 11:22:10 PST
Created attachment 590262 [details] [diff] [review]
Add some more network and plugin related SAMPLE_LABELs

This helps split Input::OnInputStreamReady
Comment 1 Benoit Girard (:BenWa) 2012-01-20 11:27:41 PST
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] 2012-01-23 13:59:43 PST
Created attachment 590863 [details] [diff] [review]
v2
Comment 3 Jeff Muizelaar [:jrmuizel] 2012-01-24 19:49:12 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ae43e3c400b
Comment 5 Olli Pettay [:smaug] 2012-01-26 06:16:02 PST
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) 2012-01-26 06:35:44 PST
(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] 2012-01-26 06:59:08 PST
(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] 2012-02-11 05:29:50 PST
So, any chance to get the ELM part fixed.

Note You need to log in before you can comment on or make changes to this bug.