Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Add some more network and plugin related SAMPLE_LABELs

RESOLVED FIXED in mozilla12

Status

()

Core
General
RESOLVED FIXED
6 years ago
6 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

6 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

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

Comment 3

6 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: 6 years ago
Resolution: --- → FIXED

Comment 5

6 years ago
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

6 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.

Comment 8

6 years ago
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.