The default bug view has changed. See this FAQ.

Add some more network and plugin related SAMPLE_LABELs

RESOLVED FIXED in mozilla12

Status

()

Core
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jrmuizel, Assigned: jrmuizel)

Tracking

unspecified
mozilla12
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 590262 [details] [diff] [review]
Add some more network and plugin related SAMPLE_LABELs

This helps split Input::OnInputStreamReady
Attachment #590262 - Flags: review?(bgirard)
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.
Attachment #590262 - Flags: review?(bgirard) → review+
(Assignee)

Comment 2

5 years ago
Created attachment 590863 [details] [diff] [review]
v2
Attachment #590262 - Attachment is obsolete: true
(Assignee)

Comment 3

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ae43e3c400b
Assignee: nobody → jmuizelaar
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/6ae43e3c400b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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?
(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.
(Assignee)

Comment 7

5 years ago
(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.
So, any chance to get the ELM part fixed.
You need to log in before you can comment on or make changes to this bug.