Remove occasional redundant Push/Pop in FlexibleShift helper

RESOLVED FIXED in Firefox 67

Status

()

enhancement
P3
normal
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: mgaudet, Assigned: kellykc72, Mentored)

Tracking

({good-first-bug})

Trunk
mozilla67
Points:
---

Firefox Tracking Flags

(firefox67 fixed)

Details

(Whiteboard: [lang=c++])

Attachments

(1 attachment)

In Bug 1526024 the FlexibleShift helper was rewritten to fix correctness, with tests added.

A side effect of the new helper however is that when shift is already in register ecx, we'll generate a push/pop pair that are redundant, as that register already will not get clobbered.

This bug is about making that push conditional.

A patch for this bug should be done when it passes the jsapi-tests unit test framework for subtests testJitMacroAssembler_flexible*shift. A bonus would be if you used masm.breakpoint() and a debugger to inspect the emitted instructions to verify the push and pop are elided.

Priority: -- → P3
(Assignee)

Comment 1

3 months ago

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #0)

In Bug 1526024 the FlexibleShift helper was rewritten to fix correctness, with tests added.

A side effect of the new helper however is that when shift is already in register ecx, we'll generate a push/pop pair that are redundant, as that register already will not get clobbered.

This bug is about making that push conditional.

A patch for this bug should be done when it passes the jsapi-tests unit test framework for subtests testJitMacroAssembler_flexible*shift. A bonus would be if you used masm.breakpoint() and a debugger to inspect the emitted instructions to verify the push and pop are elided.

Hi, This was tagged 'Good-first-bug', I was hopping I could be assigned to it, but I do not see the 'Take' button. Please advice

(Assignee)

Updated

3 months ago
Flags: needinfo?(mgaudet)

Hi, thank you for expressing interest in working on this bug. For first bugs like this we usually like to keep them open in case someone disappears after expressing interest - but don't worry, it's just a formality!

To be honest, this might be more of a good second bug, as the change here does not sound trivial. But if you don't mind a challenge, I'd recommend getting started by checking out the build documentation. There's [1] for Firefox in general, and [2] for SpiderMonkey (the JavaScript engine) in particular. Good luck!

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation

(Assignee)

Comment 3

3 months ago

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #2)

Hi, thank you for expressing interest in working on this bug. For first bugs like this we usually like to keep them open in case someone disappears after expressing interest - but don't worry, it's just a formality!

To be honest, this might be more of a good second bug, as the change here does not sound trivial. But if you don't mind a challenge, I'd recommend getting started by checking out the build documentation. There's [1] for Firefox in general, and [2] for SpiderMonkey (the JavaScript engine) in particular. Good luck!

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation

Thank you very much, I'll take a look at these resources! Best Regards

(Assignee)

Comment 4

3 months ago

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #2)

Hi, thank you for expressing interest in working on this bug. For first bugs like this we usually like to keep them open in case someone disappears after expressing interest - but don't worry, it's just a formality!

To be honest, this might be more of a good second bug, as the change here does not sound trivial. But if you don't mind a challenge, I'd recommend getting started by checking out the build documentation. There's [1] for Firefox in general, and [2] for SpiderMonkey (the JavaScript engine) in particular. Good luck!

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation

Also, thank you for explaining :)

(Assignee)

Comment 5

3 months ago

[Update:] Successfully built Firefox for Desktop. Proceeding to build SpiderMonkey

(Reporter)

Comment 6

3 months ago

Hey kellykc72, sounds like you're started up. Let me know if you have questions.

This is indeed a slightly more challenging bug, but because it's in a test suite, I think it's not too bad compared to other JIT bugs :D

(And thanks so much Emanuel for jumping in on this!)

Flags: needinfo?(mgaudet)
(Assignee)

Comment 7

3 months ago

Haha, challenges are always fun. Just getting around to completing the SpiderMonkey build. I'll keep you all posted!

(Assignee)

Comment 8

3 months ago

SpiderMonkey Build completed. I shall proceed to use searchfox to find the associated code. I'll ping back if i need more information. Thanks

(Assignee)

Comment 9

3 months ago

Checking that inline void FlexibleShift32(MacroAssembler&, Register, Register, bool, bool) is the function to be fixed. Is there a 64bit version? excuse me if that's a silly question.

(Reporter)

Comment 10

3 months ago

You've got the right function!

(Assignee)

Comment 11

3 months ago

Sweeet! I'll read up, get some context about the code, the bug what I'm thinking and you can verify and point me in the right direction. Thank you

(Assignee)

Comment 12

3 months ago

Hey, so I wrapped preserve.add(ecx); in a condition. However, I want to confirm that this statement from your original report "A patch for this bug should be done when it passes the jsapi-tests unit test framework for subtests testJitMacroAssembler_flexible*shift." means my fix must pass testJitMacroAssembler_flexible*shift before it will be accepted.

Flags: needinfo?(mgaudet)
(Assignee)

Comment 13

3 months ago

In the meantime, I will research how to test and debug the JIT engine. Thank you

(Reporter)

Comment 14

3 months ago

Yep. That's correct.

If you've not done so already, after you build the shell, there should be in dist/bin a jsapi-tests binary; you can run the shift tests by just doing ./dist/bin/jsapi-tests flexible (ok, I mean, that also runs flexibleDivMod and flexibleRemainder but these don't take too long).

It might be cool/educational (up to you) to use masm.breakpoint() to cause a trap to happen when the case you're fixing happens so you can look at the assembly code and see how it changes before and after your patch.

Flags: needinfo?(mgaudet)
(Assignee)

Comment 15

3 months ago

Oh nice, I actually came here to ask about what you just answered! So I found the binary but in other to find the usage, I was searching for an entry point. I saw a main in tests.cpp, but from this command you just gave ./dist/bin/jsapi-tests flexible, it appears I had the wrong entry point. I guess the broader question will be 'are there any documentation on how the tests are structured/run?'

Oh yeah I was gonna do the breakpoint stuff. But before that. Please confirm my workflow, I feel its supposed to be a lot simpler.

  1. Modified the appropriate function
  2. Executed the configure --enable-debug --disable-optimize --enable-nspr-build --enable-build-backends=CompileDB,RecursiveMake I don't think I should have done this. Please confirm
  3. Executed mozmake I think this is all I should have needed to do, although it seemed to have done way more stuff than I was expecting (considering I have run it before and only made a little change)

Let me go checkout the masm.breakpoint stuff.

Flags: needinfo?(mgaudet)
(Assignee)

Comment 16

3 months ago

Also, could you assist me with the debugging workflow? I've added the debug code, I just don't know how to debug. I tried looking at https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Hacking_Tips. Thanks a lot!

(Assignee)

Comment 17

3 months ago

(In reply to kellykc72 from comment #16)

Also, please could you assist me with the debugging workflow? I've added the debug code, I just don't know how to debug. I tried looking at https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Hacking_Tips. Thanks a lot!

Regarding the configure options, I don't think you need the --enable-build-backends option. The others should be fine for debugging purposes - --disable-optimize makes it slow, but without it a lot of things will get optimized out.

One thing to note: as of fairly recently, Windows shell builds are 64-bit by default (I assume you're on Windows because you mentioned mozmake). If you're adjusting the x86 JIT backend you might need a cross-compile. Just adding --target=i686-pc-mingw32 might work, though if it complains about the host configuration being unknown you might need to add --host=x86_64-pc-mingw32 as well.

You can pass the -s flag to mozmake just like with make to make it a bit more quiet, and -jn with n a number to make it perform multiple jobs in parallel (e.g. -j6 for 6 parallel jobs). The console output will get a bit mangled unfortunately, but it should be a lot faster.

Oh, and you might already be doing this, but it's highly recommended to build in a subdirectory. So you'd navigate to js/src, mkdir mybuild, cd mybuild and then run configure as ../configure instead.

(Reporter)

Comment 20

2 months ago

(In reply to kellykc72 from comment #16)

Also, could you assist me with the debugging workflow? I've added the debug code, I just don't know how to debug. I tried looking at https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Hacking_Tips. Thanks a lot!

(Bearing in mind I don't have a lot of Windows development experience)

It should be just a matter of attaching to jsapi-tests with a debugger; when you execute the JIT compiled code that has a masm.breakpoint() in, most debuggers should stop there. At that point you're stopped in JIT compiled code, and so you don't get symbols or line numbers. You'd then want to check the assembly by using your debuggers disassembly instructions.

At this point it sounds like you've got a patch worth looking at: Follow the instructions here to submit a patch :D

Flags: needinfo?(mgaudet)

Using Visual Studio, what you can do is go to File, Open, Project/Solution... and open jsapi-tests.exe as though it were a project. Then you can adjust the project properties to set the arguments and click the Start button to start debugging.

Alternatively you can try running devenv.exe directly with the /DebugExe parameter, but it might not be in your %PATH% (it's located in an awkward location like %ProgramFiles(x86)%\Microsoft Visual Studio\2017\Community\Common7\IDE). If you want to try it, the correct incantation is devenv -DebugExe path/to/jsapi-tests.exe [parameters] (the .exe extension is required or -DebugExe will silently fail).

(Assignee)

Comment 22

2 months ago

Thanks [:ehoogeveen] and [:mgaudet]. Let me play around with these suggestions.

(Assignee)

Comment 23

2 months ago

Hi Emanuel, thanks, using the VS17 debugger worked fine given the steps you illustrated!
Also, I didn't cross-compile yet. Do I have to? I didn't imagine there will be a drastic change in behavior, please advice.

Flags: needinfo?(emanuel.hoogeveen)
(Assignee)

Comment 24

2 months ago

Hi Matthew, I believe this is the behavior you are expecting? Please advice

shift in ECX (PUSH/POP pair ABSENT for ECX)

  00000350A6DA0092  mov         ecx,0FEEDBADh
  00000350A6DA0097  mov         eax,10h  
  00000350A6DA009C  mov         ecx,2  
  00000350A6DA00A1  int         3                            :BREAK

EAX = 16, ECX = 2 // in DECIMAL
  00000350A6DA00A2  shl         eax,cl                       :16 << 2
EAX = 64, ECX = 2
  00000350A6DA00A4  mov         r11,0CFFB1FFA68h             :*(0CFFB1FFA68h) is 64
EAX = 64, ECX = 2, R11 = 0CFFB1FFA68h, [0CFFB1FFA68h] = 64
  00000350A6DA00AE  cmp         dword ptr [r11],eax          :*(0CFFB1FFA68h) == 64 <=> Equal
EAX = 64, ECX = 2, R11 = 0CFFB1FFA68h, [0CFFB1FFA68h] = 64
  00000350A6DA00B1  jne         00000350A6DA00CF             :Jump not taken
EAX = 64, ECX = 2, R11 = 0CFFB1FFA68h, [0CFFB1FFA68h] = 64
  00000350A6DA00B7  mov         r11,0CFFB1FFA70h             :*(0CFFB1FFA70h) is 2
EAX = 64, ECX = 2, R11 = 0CFFB1FFA70h, [0CFFB1FFA70h] = 2
  00000350A6DA00C1  cmp         dword ptr [r11],ecx          :*(0CFFB1FFA70h) == 2 <=> Equal
EAX = 64, ECX = 2, R11 = 0CFFB1FFA70h, [0CFFB1FFA70h] = 2
  00000350A6DA00C4  jne         00000350A6DA0338             :Jump not taken
EAX = 64, ECX = 2, R11 = 0CFFB1FFA70h, [0CFFB1FFA70h] = 2
  00000350A6DA00CA  jmp         00000350A6DA0F58             :Unconditional Jump taken

Random value in ECX (PUSH/POP pair PRESENT for ECX)

  00000350A6DA0F58  mov         ecx,0FEEDBADh  
  00000350A6DA0F5D  mov         eax,10h  
  00000350A6DA0F62  mov         edx,2  
  00000350A6DA0F67  int         3                            :BREAK

EAX = 16, EDX = 2, ECX = 0FEEDBADh
  00000350A6DA0F68  push        rcx                          :Save ECX (it'll be clobbered)
EAX = 16, EDX = 2, ECX = 0FEEDBADh
  00000350A6DA0F69  mov         rcx,rdx                      :Overwrite RCX content with RDX
EAX = 16, EDX = 2, ECX = 2
  00000350A6DA0F6C  shl         eax,cl                       :16 << 2
EAX = 64, EDX = 2, ECX = 2
  00000350A6DA0F6E  pop         rcx                          :Reset ECX using saved value
EAX = 64, EDX = 2, ECX = 0FEEDBADh
  00000350A6DA0F6F  mov         r11,0CFFB1FFA68h             :*(0CFFB1FFA68h) is 64
EAX = 64, EDX = 2, ECX = 0FEEDBADh, R11 = 0CFFB1FFA68h
  00000350A6DA0F79  cmp         dword ptr [r11],eax          :*(0CFFB1FFA68h) == 64 <=> Equal
EAX = 64, EDX = 2, ECX = 0FEEDBADh, R11 = 0CFFB1FFA68h
  00000350A6DA0F7C  jne         00000350A6DA0FAD             :Jump is not taken
EAX = 64, EDX = 2, ECX = 0FEEDBADh, R11 = 0CFFB1FFA68h
  00000350A6DA0F82  mov         r11,0CFFB1FFA70h             :*(0CFFB1FFA70h) is 2
EAX = 64, EDX = 2, ECX = 0FEEDBADh, R11 = 0CFFB1FFA70h
  00000350A6DA0F8C  cmp         dword ptr [r11],edx          :*(0CFFB1FFA70h) == 2 <=> Equal
EAX = 64, EDX = 2, ECX = 0FEEDBADh, R11 = 0CFFB1FFA70h
  00000350A6DA0F8F  jne         00000350A6DA1216             :Jump is not taken
EAX = 64, EDX = 2, ECX = 0FEEDBADh, R11 = 0CFFB1FFA70h
  00000350A6DA0F95  mov         r11,7FF7F1F8ABE8h            :*(7FF7F1F8ABE8h) is 0FEEDBADh 
EAX = 64, EDX = 2, ECX = 0FEEDBADh, R11 = 7FF7F1F8ABE8h
  00000350A6DA0F9F  cmp         dword ptr [r11],ecx          :*(7FF7F1F8ABE8h) == 0FEEDBADh <=> Equal
EAX = 64, EDX = 2, ECX = 0FEEDBADh, R11 = 7FF7F1F8ABE8h
  00000350A6DA0FA2  jne         00000350A6DA147F             :Jump is not taken
EAX = 64, EDX = 2, ECX = 0FEEDBADh, R11 = 7FF7F1F8ABE8h
  00000350A6DA0FA8  jmp         00000350A6DA1E33             :Unconditional Jump taken
Flags: needinfo?(mgaudet)
(Assignee)

Comment 25

2 months ago

(In reply to kellykc72 from comment #23)

Hi Emanuel, thanks, using the VS17 debugger worked fine given the steps you illustrated!
Also, I didn't cross-compile yet. Do I have to? I didn't imagine there will be a drastic change in behavior, please advice.

Actually I tried to compile with those options and it ended in chaos haha

(Assignee)

Comment 26

2 months ago

(In reply to kellykc72 from comment #25)

(In reply to kellykc72 from comment #23)

Hi Emanuel, thanks, using the VS17 debugger worked fine given the steps you illustrated!
Also, I didn't cross-compile yet. Do I have to? I didn't imagine there will be a drastic change in behavior, please advice.

Actually I tried to configure with those options and it ended in chaos haha

I'm probably missing something but ...

In file included from d:/firefox/js/src\jit/LIR.h:1806d:/firefox/js/src\jit/LIR.h:
(715,d:/firefox/js/src\jit/x86/LIR-x86.h(209,6314))::  noteerror: : expanded fromno  macromember  'LIR_HEADER'named
 'WasmAtomicStoreI64'  static constexpr LNode::Opcode classOpcode = LNode::Opcode::opcode;
in 'js::jit::LNode::Opcode'                                               ~~~~~~~~~~~~~~~^

  LIR_HEADER(WasmAtomicStoreI64);
  ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
d:/firefox/js/src\jit/LIR.h(715,63): note: expanded from macro 'LIR_HEADER'
  static constexpr LNode::Opcode classOpcode = LNode::Opcode::opcode;
                                               ~~~~~~~~~~~~~~~^
In file included from d:/firefox/js/src/build_DBG.OBJ/js/src/wasm/Unified_cpp_js_src_wasm1.cpp:47:
In file included from d:/firefox/js/src/wasm/WasmIonCompile.cpp:23:
In file included from d:/firefox/js/src\jit/CodeGenerator.h:16:
In file included from d:/firefox/js/src\jit/x86/CodeGenerator-x86.h:10:
In file included from d:/firefox/js/src\jit/x86-shared/CodeGenerator-x86-shared.h:10:
In file included from d:/firefox/js/src\jit/shared/CodeGenerator-shared.h:16:
In file included from d:/firefox/js/src\jit/LIR.h:1806:
d:/firefox/js/src\jit/x86/LIR-x86.h(284,14): error: no member named 'WasmAtomicBinopI64' in 'js::jit::LNode::Opcode'
  LIR_HEADER(WasmAtomicBinopI64);
  ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
d:/firefox/js/src\jit/LIR.h(715,63): note: expanded from macro 'LIR_HEADER'
  static constexpr LNode::Opcode classOpcode = LNode::Opcode::opcode;
                                               ~~~~~~~~~~~~~~~^
In file included from d:/firefox/js/src/vm/Interpreter.cpp:56:
In file included from d:/firefox/js/src\jit/JitFrames-inl.h:12:
In file included from d:/firefox/js/src\jit/LIR.h:1806:
d:/firefox/js/src\jit/x86/LIR-x86.h(233,14): error: no member named 'WasmCompareExchangeI64' in 'js::jit::LNode::Opcode'
  LIR_HEADER(WasmCompareExchangeI64);
  ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~
d:/firefox/js/src\jit/LIR.h(715,63): note: expanded from macro 'LIR_HEADER'
  static constexpr LNode::Opcode classOpcode = LNode::Opcode::opcode;
                                               ~~~~~~~~~~~~~~~^
In file included from d:/firefox/js/src/vm/Interpreter.cpp:56:
In file included from d:/firefox/js/src\jit/JitFrames-inl.h:12:
In file included from d:/firefox/js/src\jit/LIR.h:1806:
d:/firefox/js/src\jit/x86/LIR-x86.h(261,14): error: no member named 'WasmAtomicExchangeI64' in 'js::jit::LNode::Opcode'
  LIR_HEADER(WasmAtomicExchangeI64);
  ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
d:/firefox/js/src\jit/LIR.h(715,63): note: expanded from macro 'LIR_HEADER'
  static constexpr LNode::Opcode classOpcode = LNode::Opcode::opcode;
                                               ~~~~~~~~~~~~~~~^
In file included from d:/firefox/js/src/vm/Interpreter.cpp:56:
In file included from d:/firefox/js/src\jit/JitFrames-inl.h:12:
In file included from d:/firefox/js/src\jit/LIR.h:1806:
d:/firefox/js/src\jit/x86/LIR-x86.h(284,14): error: no member named 'WasmAtomicBinopI64' in 'js::jit::LNode::Opcode'
  LIR_HEADER(WasmAtomicBinopI64);
  ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
d:/firefox/js/src\jit/LIR.h(715,63): note: expanded from macro 'LIR_HEADER'
  static constexpr LNode::Opcode classOpcode = LNode::Opcode::opcode
(Assignee)

Comment 27

2 months ago

Oh I forgot the test output

$ ./dist/bin/jsapi-tests.exe flexible
testJitMacroAssembler_flexibleLshift
TEST-PASS | testJitMacroAssembler_flexibleLshift | ok
testJitMacroAssembler_flexibleRshiftArithmetic
TEST-PASS | testJitMacroAssembler_flexibleRshiftArithmetic | ok
testJitMacroAssembler_flexibleRshift
TEST-PASS | testJitMacroAssembler_flexibleRshift | ok
testJitMacroAssembler_flexibleQuotient
TEST-PASS | testJitMacroAssembler_flexibleQuotient | ok
testJitMacroAssembler_flexibleRemainder
TEST-PASS | testJitMacroAssembler_flexibleRemainder | ok
testJitMacroAssembler_flexibleDivMod
TEST-PASS | testJitMacroAssembler_flexibleDivMod | ok

Passed: ran 6 tests.
(Reporter)

Comment 28

2 months ago

This code is shared between x86 and x86-64 backends, so no need to cross compile!

Flags: needinfo?(emanuel.hoogeveen)
(Reporter)

Comment 29

2 months ago

I think the behaviour looks right. Definitely time to submit a patch :)

Flags: needinfo?(mgaudet)
(Assignee)

Comment 30

2 months ago

sweeet!!

FWIW cross compilation seems to be broken for me at the moment, at least using mozmake and configure (mach seems okay). I'll have to file a bug, but no need to worry about it here.

(Assignee)

Comment 32

2 months ago

Update: busy week(end), just getting around setting up Phabricator. Didn't know earlier, should've set it up with everything else before starting. Sorry for the delay

(Assignee)

Comment 33

2 months ago

The ECX register must contain the SHIFT amount. Before this change, ECX will always be push/pop during a SHIFT operation. However, ECX should only be push/pop if it's original contents are going to be overwritten. We now only push/pop ECX if it contains some arbituary value and hence will be overwritten by the SHIFT operation.

Attachment #9049858 - Attachment description: make push/pop pair for ecx conditional on previous ecx content → Make push/pop pair for ecx conditional on previous ecx content
(Reporter)

Comment 34

2 months ago

Thanks so much for your contribution! If you'd like to keep rolling, please, feel free to look for more mentored bugs in SpiderMonkey.

Perhaps give Bug 1531479 a look.

(Pulsebot would normally link to the landed code, but it seems to be sleeping right now, so:

https://hg.mozilla.org/integration/autoland/rev/5ec94981916f0660a2cd2348ef41a10ffc4fca93

)

(Reporter)

Updated

2 months ago
Assignee: nobody → kellykc72
(Assignee)

Comment 35

2 months ago

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #34)

Thanks so much for your contribution! If you'd like to keep rolling, please, feel free to look for more mentored bugs in SpiderMonkey.

Perhaps give Bug 1531479 a look.

(Pulsebot would normally link to the landed code, but it seems to be sleeping right now, so:

https://hg.mozilla.org/integration/autoland/rev/5ec94981916f0660a2cd2348ef41a10ffc4fca93

)

Thanks to you and :ehoogeveen for all the assistance and yes, I'll keep rolling. Let me take a look :)

(Assignee)

Comment 36

2 months ago

Hi, please confirm no further action's are required from me for this. I read something about landing but I suppose you as the reviewer is responsible for that?

Flags: needinfo?(mgaudet)
(Assignee)

Comment 37

2 months ago

Oh nvm, I guess that's the link you sent regarding Pulsebot. Thanks!

Flags: needinfo?(mgaudet)
(Reporter)

Comment 38

2 months ago

You're all done. What's happening now is the code has landed on our integration branch 'autoland'; it's been tested through our continuous integration system; you can see the results here.

Next up, at some point in the not too distant future the Mozilla Sheriffs will merge autoland into mozilla-central, the repository from which our nightly builds are built. At that point, this bug will get marked as RESOLVED by the sheriff at the merge time.

Then, a few hours after that your code will be in a copy of Firefox Nightly that tens of thousands run every day :)

Party time: 🎉🎉🎉

(Assignee)

Comment 39

2 months ago

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #38)

You're all done. What's happening now is the code has landed on our integration branch 'autoland'; it's been tested through our continuous integration system; you can see the results here.

Next up, at some point in the not too distant future the Mozilla Sheriffs will merge autoland into mozilla-central, the repository from which our nightly builds are built. At that point, this bug will get marked as RESOLVED by the sheriff at the merge time.

Then, a few hours after that your code will be in a copy of Firefox Nightly that tens of thousands run every day :)

Party time: 🎉🎉🎉

Haha, thanks for the clarification! Already looking forward to my next contribution!

(In reply to kellykc72 from comment #35)

Thanks to you and :ehoogeveen for all the assistance and yes, I'll keep rolling. Let me take a look :)

No problem, congrats on your first contribution! I'm glad you had a positive experience and I'm looking forward to seeing what you do in the future :)

Comment 41

2 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.