Closed Bug 1092547 Opened 10 years ago Closed 9 years ago

IonMonkey: Implement MathFunction(Log) recover instruction

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: nbp, Assigned: babhishek21, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug][lang=c++])

Attachments

(3 files, 5 obsolete files)

Implement MathFunction(Log) in js/src/jit/Recover.cpp.
See Bug 1003801 comment 0 for explanation.
Blocks: 1079384
Whiteboard: [good first bug][lang=c++]
Hi,

This is for basic LOG (natural logarithm (base e) of a number) not LOG2E or LOG10E, correct?

Thank you,
Lawrence Abadier
(In reply to Nicolas B. Pierron [:nbp] from comment #6)
> (In reply to Peter from comment #3)
> > Yep, sorry, I forgot to comment. I can share my current work if it can help
> > Jarda.
> > Otherwise, I'm pretty confident I will submit a patch by next monday.
> 
> Are you still working on this feature?
> 

No, I didn't work on it since the bug squashing party and I don't think I will be able to finish it. Sorry :(

So this bug is free to be taken by someone else.
Hello! I am new to FOSS and would like to work on this bug. Can someone guide/mentor me [:nbp] ? I am a total newbie and might require some help.

PS: From what I've inferred, am I supposed to write
1. case Log: //Statements here in the MMathFunction:: @ js/src/jit/Recover.cpp#891
2. case MMathFunction::Log: {//Statements here} for RMathFunction::recover @ js/src/jit/Recover.cpp#915 ?

Thank you in advance. Possibly my first bug-fix request.
(In reply to babhishek21 from comment #11)
> Hello! I am new to FOSS and would like to work on this bug. Can someone
> guide/mentor me [:nbp] ? I am a total newbie and might require some help.

Sure, I can mentor you.

> PS: From what I've inferred, am I supposed to write
> 1. case Log: //Statements here in the MMathFunction:: @
> js/src/jit/Recover.cpp#891
> 2. case MMathFunction::Log: {//Statements here} for RMathFunction::recover @
> js/src/jit/Recover.cpp#915 ?

And,
3. Adding a test case in jit-test/tests/ion/dce-with-rinstructions.js

And, with the test case, you will be able to see that there is one other thing to patch to make this code used ;)
(In reply to Lawrence Abadier from comment #8)
> This is for basic LOG (natural logarithm (base e) of a number) not LOG2E or
> LOG10E, correct?

This is what Math.log returns (js::math_log), which looks like the base "e" log.
Attached patch Bug1092547_Math_Log.diff (obsolete) — Splinter Review
Hi,

I had worked a little bit on this a few weeks back, but waited for a response before continuing; mind you I don't want to take anything away from all the others that came in asking to work on this.

I'm having a few issues with testing atm but thought I would upload my patch till I have the testing fixed.

As for the patch I didnt know if: if (!js::math_log(cx, arg, &result)) was the correct one, couldn't find an alternative handle to use.

Thank you,
Lawrence Abadier
Attachment #8546226 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8546226 [details] [diff] [review]
Bug1092547_Math_Log.diff

Review of attachment 8546226 [details] [diff] [review]:
-----------------------------------------------------------------

Does the test pass locally?

::: js/src/jit/Recover.cpp
@@ +850,5 @@
> +    switch (function_) {
> +      case Round:
> +        writer.writeUnsigned(uint32_t(RInstruction::Recover_Round));
> +        return true;
> +      case Sin:

AFAIK, Sin was is already in the tree, are you based on the latest version?

@@ +854,5 @@
> +      case Sin:
> +        writer.writeUnsigned(uint32_t(RInstruction::Recover_MathFunction));
> +        writer.writeByte(function_);
> +        return true;
> +       case Log:

style-nit: Wrong indentation.

::: js/src/jit/Recover.h
@@ +43,5 @@
>      _(MinMax)                                   \
>      _(Abs)                                      \
>      _(Sqrt)                                     \
>      _(Atan2)                                    \
> +    _(Log)                                    \

Why adding this line here?
Attachment #8546226 - Flags: review?(nicolas.b.pierron)
Hello. Thanks Lawrence. I am facing some issues in my Windows build environment (js shell kept reporting missing dll files REF: bug 1003801, comment 0 ). 

To [:nbp] I have a working directory of mozilla-central. However even after repeated mercurial pull/update cycles, the contents on some files in my local working directory do not match with the web dxr of Mozilla. 

For instance: js/src/jit/Recover.cpp#887 in web dxr already has a `case Sin:` implementation. My working copy does not. In fact, my working copy does not even have `RMathFunction::RMathFunction` and `RMathFunction::recover` implementations (which are available at #896, #902 in the web dxr). Does the web dxr also display non-committed changes? In that case should I keep working with the my local working directory files?

Sorry, if posted in the wrong place. 
REF: web dxr = https://dxr.mozilla.org/mozilla-central/source/js/src/jit/Recover.cpp
Hi Nicolas,

Im getting the same issue babh is, I simply added sin because dxr did; however, even after the most recent pull my files don't match dxr.

In addition to that, I cant pulled 2 different copies and both are missing nss3.dll. So for one build I cant actually make a JS Shell, and for the one that has a JS Shell it gives me missing DLL. (So cant actually test anything, it makes all tests fail).

I have researched the issue and not sure what to do, any guidance on the matter?
dxr is lagging behind mozilla-central, which is the repository where you can make you work most of the time.  We push on mozilla-inbound, which is later merge to mozilla-central [1].

Can you check that your .hg/hgrc is referring to mozilla-central?

[1] http://hg.mozilla.org/mozilla-central/
Hi,

So I checked my hgrc and it is pointing to central:

>[paths]
>default = http://hg.mozilla.org/mozilla-central

Additionally, my nss3.dll exist in both pull attempts, yet I cannot launch js.exe nor build it from scratch.
Hello!
I managed to get a better pull this time (after 4 fails). It matches the dxr. It also matched up all right with some mxr searches that I ran. So I did another build. This time I got missing nspr*.dll. Now I realise I was building on a Non-POSIX platform. Will fix that.

Ironically Firefox releases don't ship with NSPR (nspr*.dll) dlls for Windows now. :/
If you're just building the js shell, you should configure with --enable-nspr-build so it builds nspr for you. Building all of Firefox with mach will also build nspr, but of course this takes a lot longer.
Hi Emanuel. 
I actually built the complete firefox nightly using mach. JS shell (js.exe) still wouldn't run. That time it was missing ici*.dll and nss*.dll files missing. I guess this time, it didn't reach that point (aborted at NSPR check). I am thinking about using `--enable-nspr-build` or doing the build on Linux. 

BTW: I have this question. Is NSPR (pre-built) already available in mozilla-build-setup for Windows? Or do I need to checkout and build NSPR myself before using `--enable-nspr-build`? (as suggested by https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSPR )
As long as you're using the latest MozillaBuild's [1] start-shell-msvc2013.bat with Visual Studio 2013 and Windows SDK 8.1, things should Just Work™. If you're missing dlls, that suggests the build didn't finish successfully. To build just a debug shell, use something like

cd js/src
autoconf-2.13
mkdir build-debug
cd build-debug
../configure --enable-nspr-build --enable-debug
mozmake -s -j8

[1] https://wiki.mozilla.org/MozillaBuild
Oh, and to answer your question: the NSPR sources are included in the nsprpub directory of the mozilla-central tree. IIRC this is compiled into a library that the shell and Firefox itself are statically linked to during the build process.
Thank you Emanuel. But the shell still won't run after enabling NSPR. nspr*.dll are still missing. I think it might be because JS shell is not "INSTALLED" into the Windows system. All I have done is build. It still doesn't know where to access some system files from (this is my conjecture).

So perhaps I require an equivalent of `make install` for Windows. `mozmake` only seems to be building and not pointing to dependencies.
Flags: needinfo?(emanuel.hoogeveen)
I don't really know what to tell you, other than to ensure that you're trying to open the correct js shell (i.e. not one from a failed build). Using the commands I provided, your freshly built shell should be in |js/src/build-debug/dist/bin|. This directory should also have nspr4.dll in it (I guess the shell or the tests use dynamic linking). Using |./mach build| should create the shell in |obj-i686-pc-mingw32/dist/bin|.

If this still doesn't work, make sure none of the steps are halting early with an error. You can use |./mach clobber| to start fresh, or just delete |js/src/build-debug| if you're building the shell. Otherwise you're going to have to ask a build peer. You can also try setting up a Linux VM and use that, but this really should just work.
Flags: needinfo?(emanuel.hoogeveen)
Hi Emanuel. 
Sorry. I screwed up big time :( I tried running the shell from:
1. build-debug/js/src/js.exe 
2. build-debug/js/src/shell/js.exe (I was pointed here by IRC)
Sorry for being an a**. I should have realised that the binary should always be in the dist/bin folders.

I believe running `js dce-with-rinstructions.js` should run the tests? 
Alright, I'll get to work on it tomorrow. (This 11-hour time difference between India and US is screwing up my schedule.)
Flags: needinfo?(emanuel.hoogeveen)
Yes, although this file is located in |js/src/jit-test/tests/ion|, so from your build directory you'll want something like

`dist/bin/js.exe ../jit-test/tests/ion/dce-with-rinstructions.js`

Alternatively, to run it using the jit-test framework, use something like

`../jit-test/jit_test.py dist/bin/js.exe dce-with-rinstructions`

But I don't really know what you need to run to test your work in this bug - you can probably find more information in previous recover instruction bugs, or ask nbp if you get stuck :)
Flags: needinfo?(emanuel.hoogeveen)
I will have to make a whole new directory and build from scratch. Thus far, I cant test nor build js shell in two directories. Running: start-shell-msvc2012-x64.bat , with vs2012. not sure if this is the issue as I have tested my code using the same method before. We will see how it goes with a new pull.
Support for Visual Studio 2012 was very recently removed; you now need Visual Studio 2013 with at least update 3. Also, only use start-shell-msvc2013-x64.bat if you want to make a 64-bit *build*. For 32-bit builds, use start-shell-msvc2013.bat regardless of whether you're running 32-bit or 64-bit Windows. If you *do* want a 64-bit build, you need to pass |--target=x86_64-pc-mingw32 --host=x86_64-pc-mingw32| to configure, or add

ac_add_options --target=x86_64-pc-mingw32
ac_add_options --host=x86_64-pc-mingw32

to your .mozconfig if you're using mach.
Hello Lawrence. I think the problem is using x64 version of start-shell-msvc2012-***.bat. Best use the 32 bit version, or follow [:ehoogeveen]'s advice. The x64 version is known to break builds.
(In reply to Abhishek Bhattacharya from comment #31)
> Hello Lawrence. I think the problem is using x64 version of
> start-shell-msvc2012-***.bat. Best use the 32 bit version, or follow
> [:ehoogeveen]'s advice. The x64 version is known to break builds.

Well yes, it would if you don't follow comment 30. The -x64 .bat is specifically for x64 builds. So yes, you need a .mozconfig to match.
Alright, made some changes on an updated pull.
Specifically:
1. Added |rlog_number| and |rlog_object| tests to /js/src/jit-test/tests/ion/dce-with-rinstructions.js#1064
2. Added |case Log:| to js/src/jit/MIR.h#5614
3. Added |case Log:| to /js/src/jit/Recover.cpp#883 {both MMath and RMath} 

Could NOT run tests locally. Refer [1] I'll be grateful if someone could test this patch for me, or give me pointers on how to get IONGRAPH working for me. 

@ Lawrence: Not sure you were working on it or not. No hard feelings. Feel free to use the patch.

I also noticed that at
1. https://dxr.mozilla.org/mozilla-central/source/js/src/jit-test/tests/ion/dce-with-rinstructions.js#1057
2. https://dxr.mozilla.org/mozilla-central/source/js/src/jit-test/tests/ion/dce-with-rinstructions.js#1068

the |assertEq| should be called with {x, Math.sin(99)} for effective checking. (Since {x, Math.sin(i)} would always return true.) (This is in keeping in line with Ref:[2]) IMHO
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[1] I tried running these commands to get an iongraph log output:

1. $ IONFLAGS=logs js/src/build_DBG.OBJ/dist/bin/js --ion-offthread-compile=on js/src/jit-test/tests/ion/dce-with-rinstructions.js (Apparently --ion-parallel-compile=off is deprecated)
Got Error messages "Can't log script dce-with-instructions.js:1 (Compiled on background thread)"

2. [Refer Bug 1012632, comment 12] 
$ IONFLAGS=logs,bailouts ./dist/bin/js --ion-offthread-compile=on ../jit-test/tests/ion/dce-with-rinstructions.js
(I also tried --ion-offthread-compile=off) Got error messages like:
> [IonBailouts] Took invalidation bailout! Snapshot offset: 171
> [IonBailouts]  bailing from bytecode: nop, MIR: constant [0], LIR: osipoint [47]

Both gave error messages regarding not being able to log. End result: No PNG. NO json. No log.
NOTE: My environment is Windows Pro 8.1. Could this be causing any problems? I'm using mozilla-build-tools 1.10
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[2] https://dxr.mozilla.org/mozilla-central/source/js/src/jit-test/tests/ion/dce-with-rinstructions.js#19
Attachment #8549026 - Flags: review?(nicolas.b.pierron)
(In reply to Abhishek Bhattacharya from comment #31)
> Hello Lawrence. I think the problem is using x64 version of
> start-shell-msvc2012-***.bat. Best use the 32 bit version, or follow
> [:ehoogeveen]'s advice. The x64 version is known to break builds.

This actually fixed my issue, I been stuck for a week trying to find out why nothing was working. (Even downloaded vs2013) I thought I had a 64 build. Your patch looks good, no worries on you working on it; I just wanted to contribute something while everything was broken, hope it somewhat helped you out. I will finish the latest build and test your code out, I could never get ION graph to work for me previously.

Lawrence,
(In reply to Abhishek Bhattacharya from comment #33)
> Could NOT run tests locally. Refer [1] I'll be grateful if someone could
> test this patch for me, or give me pointers on how to get IONGRAPH working
> for me. 

On windows, you might have to specify the path to ion.json file which is created in the JIT_SPEW_DIR.  Unfortunately, I never tried iongraph on Windows, but you need python (that you should already have if you used "mach") and graphviz.
Comment on attachment 8549026 [details] [diff] [review]
bug_1092547: rev_1 (Specific rectifications to patch: Bug1092547_Math_Log.diff)

Review of attachment 8549026 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good!

::: js/src/jit/Recover.cpp
@@ +888,5 @@
>          writer.writeUnsigned(uint32_t(RInstruction::Recover_MathFunction));
>          writer.writeByte(function_);
>          return true;
> +      case Log:
> +        writer.writeUnsigned(uint32_t(RInstruction::Recover_MathFunction));

nit: As this is doing the same thing as the Sin case, we might want to avoid the code duplication and have multiple case for a block ;)
Attachment #8549026 - Flags: review?(nicolas.b.pierron) → review+
(In reply to Nicolas B. Pierron [:nbp] from comment #36)
> Review of attachment 8549026 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good!
> 
> ::: js/src/jit/Recover.cpp
> @@ +888,5 @@
> >          writer.writeUnsigned(uint32_t(RInstruction::Recover_MathFunction));
> >          writer.writeByte(function_);
> >          return true;
> > +      case Log:
> > +        writer.writeUnsigned(uint32_t(RInstruction::Recover_MathFunction));
> 
> nit: As this is doing the same thing as the Sin case, we might want to avoid
> the code duplication and have multiple case for a block ;)

Haha. This is an epic face-palm moment for my mistake. I'll fix it pronto.
Fixed the code duplication "nit" warning. 

Still haven't been able to test locally. Will try to do it ASAP on a Linux platform.

BTW what does this "nit" (in code review) mean?
Attachment #8549026 - Attachment is obsolete: true
Attachment #8549603 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8549603 [details] [diff] [review]
bug_1092547: rev_2 (Fix code duplication)

Review of attachment 8549603 [details] [diff] [review]:
-----------------------------------------------------------------

This sounds good.
Attachment #8549603 - Flags: review?(nicolas.b.pierron) → review+
(In reply to Abhishek Bhattacharya from comment #38)
> BTW what does this "nit" (in code review) mean?

http://en.wikipedia.org/wiki/Nitpicking

Basically this is to indicate areas where the code can be improved, and we expect you to do that or to reply to justify why not.  These are usually minor fixes which are not invalidating an r+ ;)
(In reply to Nicolas B. Pierron [:nbp] from comment #39)
> Comment on attachment 8549603 [details] [diff] [review]
> bug_1092547: rev_2 (Fix code duplication)
> 
> Review of attachment 8549603 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This sounds good.

So what next? Do I need to add any flags to the commit message? Or are you waiting for the IONGRAPH outputs?
Alright. So I tried to build the js shell on Linux. The build failed with this error message:

/home/quirk/dev/mozilla-central/js/src/jit/Recover.cpp:920:43: error: cannot convert ‘JS::RootedValue {aka JS::Rooted<JS::Value>}’ to ‘unsigned int’ for argument ‘2’ to ‘bool js::math_log(JSContext*, unsigned int, JS::Value*)’
         if (!js::math_log(cx, arg, &result))
                                           ^
make[3]: *** [Unified_cpp_js_src5.o] Error 1
make[3]: Leaving directory `/home/quirk/dev/mozilla-central/js/src/build_DBG.OBJ/js/src'
make[2]: *** [js/src/target] Error 2
make[2]: Leaving directory `/home/quirk/dev/mozilla-central/js/src/build_DBG.OBJ'
make[1]: *** [compile] Error 2
make[1]: Leaving directory `/home/quirk/dev/mozilla-central/js/src/build_DBG.OBJ'
make: *** [default] Error 2

It looks like there was an `unsigned int` to `bool` conversion error in the |RMathFunction::recover| case MMathFunction::Log. in Recover.cpp

Oddly enough the js shell built perfectly in Windows. 

Any pointers?
(In reply to Abhishek Bhattacharya from comment #42)
> Alright. So I tried to build the js shell on Linux. The build failed with
> this error message:
> 
> /home/quirk/dev/mozilla-central/js/src/jit/Recover.cpp:920:43: error: cannot
> convert ‘JS::RootedValue {aka JS::Rooted<JS::Value>}’ to ‘unsigned int’ for
> argument ‘2’ to ‘bool js::math_log(JSContext*, unsigned int, JS::Value*)’
>          if (!js::math_log(cx, arg, &result))
>                                            ^
> make[3]: *** [Unified_cpp_js_src5.o] Error 1
> make[3]: Leaving directory
> `/home/quirk/dev/mozilla-central/js/src/build_DBG.OBJ/js/src'
> make[2]: *** [js/src/target] Error 2
> make[2]: Leaving directory
> `/home/quirk/dev/mozilla-central/js/src/build_DBG.OBJ'
> make[1]: *** [compile] Error 2
> make[1]: Leaving directory
> `/home/quirk/dev/mozilla-central/js/src/build_DBG.OBJ'
> make: *** [default] Error 2
> 
> It looks like there was an `unsigned int` to `bool` conversion error in the
> |RMathFunction::recover| case MMathFunction::Log. in Recover.cpp
> 
> Oddly enough the js shell built perfectly in Windows. 
> 
> Any pointers?

This issue sounds legitimate, usually people add a new entry point for the function (js::math_log_handle), such that it can be called with HandleValue argument and MutableHandleValue out-param.  If this does compile on windows, then there is something extremely weird going on and we should double check that, because this would be a compiler issue.
(In reply to Nicolas B. Pierron [:nbp] from comment #43)
> (In reply to Abhishek Bhattacharya from comment #42)
> This issue sounds legitimate, usually people add a new entry point for the
> function (js::math_log_handle), such that it can be called with HandleValue
> argument and MutableHandleValue out-param.  If this does compile on windows,
> then there is something extremely weird going on and we should double check
> that, because this would be a compiler issue.

It might be a compiler issue. Also, |js::math_log_handle| does not exist at the moment. The correct handler for that goes by |js::math_log| (I checked this with "jsmath.h" and "jsmath.cpp"). 

Another weird thing happened. Apropos Comment #33, remember IONGRAPH gave weird messages (incomprehensible to me at-least). Well guess what? It outputted a "ion.cfg" and "ion.json".(They came up when I ran |hg stat|) But still no PNGs or PDFs. Also while every function in "dce-with-rinstructions.js" seems to have compiled alright, the JSON output looks blank. I've attached the pastebin links:

[1] ion.json http://babhishek21.pastebin.mozilla.org/8230890 [validity = forever]
[2] ion.cfg http://babhishek21.pastebin.mozilla.org/8230915 [validity = forever]

In the mean time if any one can run the tests for me or help me fix this problem, it would be swell.

To Nicolas [:nbp] What do you make of it?
(In reply to Abhishek Bhattacharya from comment #44)
> (In reply to Nicolas B. Pierron [:nbp] from comment #43)
> > (In reply to Abhishek Bhattacharya from comment #42)
> > This issue sounds legitimate, usually people add a new entry point for the
> > function (js::math_log_handle), such that it can be called with HandleValue
> > argument and MutableHandleValue out-param.  If this does compile on windows,
> > then there is something extremely weird going on and we should double check
> > that, because this would be a compiler issue.
> 
> It might be a compiler issue. Also, |js::math_log_handle| does not exist at
> the moment. The correct handler for that goes by |js::math_log| (I checked
> this with "jsmath.h" and "jsmath.cpp"). 
So just to make sure that that JS shell does compile on Windows, I tried to build it again. IT FAILED at the same error message. How it managed to build the first time, I can only guess.(perhaps my patch was not applied then)
And since |js::math_log_handle| does not exist, I'll try to do either of these:
1. Add a |js::math_log_handle| in "jsmath.cpp" and "jsmath.h"
2. Find a way to pass arguments safely to |js::math_log|

I prefer the first option. Will look into it.

It seems on further reading, that I need to install IONGRAPH and GraphViz to generate PNGs from ion.cfg. Is there a shorter way about it?
(In reply to Abhishek Bhattacharya from comment #44)
> Another weird thing happened. Apropos Comment #33, remember IONGRAPH gave
> weird messages (incomprehensible to me at-least). Well guess what? It
> outputted a "ion.cfg" and "ion.json".(They came up when I ran |hg stat|) But
> still no PNGs or PDFs. Also while every function in
> "dce-with-rinstructions.js" seems to have compiled alright, the JSON output
> looks blank. I've attached the pastebin links:
> 
> [1] ion.json http://babhishek21.pastebin.mozilla.org/8230890 [validity =
> forever]
> [2] ion.cfg http://babhishek21.pastebin.mozilla.org/8230915 [validity =
> forever]

iongraph is the tool which make use of ion.json to generate png files.  Also, you might want to use --ion-offthread-compile=off, such that it can log some content while it is compiling.
Attachment #8546226 - Attachment is obsolete: true
Comment on attachment 8549603 [details] [diff] [review]
bug_1092547: rev_2 (Fix code duplication)

Review of attachment 8549603 [details] [diff] [review]:
-----------------------------------------------------------------

As I agree with the compiler,
I suggest you create a js::math_log_handle function in jsmath.cpp.
Attachment #8549603 - Flags: review+ → feedback+
So I managed to fix the JS shell build error. This has resulted in some major changes in /js/src/jsmath.cpp and /js/src/jsmath.h 

Changes are specifically:
1. Added |math_log_handle| as a standard js::math_log handler. 
2. Added an extern call to above in jsmath.h

Local tests run with following results:
1. JS Shell builds perfectly with no complains
2. ION LOGS generated: ion.json (~63 MB) and ion.cfg (~23 MB)
3. ION png files generated with IONGRAPH (> 400MB, ~10K files)

This patch seems to work alright. Review required.

To Nicolas [:nbp],
I have over 10K generated PNG files. Which ones am I supposed to upload? Bug 1012632 helps very little. I can tell you that |rlog_number| and |rlog_object| are implemented at the last of all test functions. They are implemented just before |for(i in 0 to 99) { call test functions }|. This will be clear once you view the patch. So what am I supposed to upload?

And one more thing: Can you please assign this bug to me?
Attachment #8549603 - Attachment is obsolete: true
Attachment #8550390 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8550390 [details] [diff] [review]
bug_1092547: rev_3: Fixed js shell build errors (MAJOR changes)

Review of attachment 8550390 [details] [diff] [review]:
-----------------------------------------------------------------

This sounds better :)
Attachment #8550390 - Flags: review?(nicolas.b.pierron) → review+
(In reply to Abhishek Bhattacharya from comment #48)
> 2. ION LOGS generated: ion.json (~63 MB) and ion.cfg (~23 MB)
> 3. ION png files generated with IONGRAPH (> 400MB, ~10K files)

You can use IONFILTER to reduce the number of function logged when running the shell.

> To Nicolas [:nbp],
> I have over 10K generated PNG files. Which ones am I supposed to upload? Bug
> 1012632 helps very little. I can tell you that |rlog_number| and
> |rlog_object| are implemented at the last of all test functions. They are
> implemented just before |for(i in 0 to 99) { call test functions }|. This
> will be clear once you view the patch. So what am I supposed to upload?

Look at the Sink phase of the functions that you added in the test file.  Then the MathFunction instruction should be present & gray instead of blue.
Assignee: nobody → babhishek21
Status: NEW → ASSIGNED
(In reply to Nicolas B. Pierron [:nbp] from comment #50)
> (In reply to Abhishek Bhattacharya from comment #48)
> > To Nicolas [:nbp],
> > I have over 10K generated PNG files. Which ones am I supposed to upload? Bug
> > 1012632 helps very little. I can tell you that |rlog_number| and
> > |rlog_object| are implemented at the last of all test functions. They are
> > implemented just before |for(i in 0 to 99) { call test functions }|. This
> > will be clear once you view the patch. So what am I supposed to upload?
> 
> Look at the Sink phase of the functions that you added in the test file. 
> Then the MathFunction instruction should be present & gray instead of blue.
I still don't understand much, but I think this is the file you are looking for.

I'll attach the corresponding file for uceFault_log_object in another attachment (Bugzilla won't let me attach multiple files.)
Flags: needinfo?(nicolas.b.pierron)
Attachment #8551238 - Flags: review?(nicolas.b.pierron)
(In reply to Abhishek Bhattacharya from comment #51)
> I'll attach the corresponding file for uceFault_log_object in another 
> attachment (Bugzilla won't let me attach multiple files.)
There you go.
Attachment #8551239 - Flags: review?(nicolas.b.pierron)
Flags: needinfo?(nicolas.b.pierron)
Attachment #8551238 - Flags: review?(nicolas.b.pierron)
Attachment #8551239 - Flags: review?(nicolas.b.pierron)
(In reply to Abhishek Bhattacharya from comment #51)
> > Look at the Sink phase of the functions that you added in the test file. 
> > Then the MathFunction instruction should be present & gray instead of blue.
> I still don't understand much, but I think this is the file you are looking
> for.

You attached the DCE phase, not the Sink phase.  This optimization moved from the DCE (Dead Code Elimination) phase to the Sink phase which is just after DCE.

These files are the representation of the JavaScript function as view by the compiler.  At the end of each compilation phase, we log this graph in the json file when IONFLAGS=logs.  IonGraph interpret what is saved in the json file to give it a visual aspect which is easy to verify without starting a debugger.
There you go. Is this the one you need?
Attachment #8551238 - Attachment is obsolete: true
Attachment #8551259 - Flags: review?(nicolas.b.pierron)
Attachment #8551239 - Attachment is obsolete: true
Attachment #8551262 - Flags: review?(nicolas.b.pierron)
Attachment #8551259 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8551262 [details]
func228-pass17-Sink-mir.gv.png: uceFault_log_object IONGRAPH output

Do not ask for review if this is not a patch.
Attachment #8551262 - Flags: review?(nicolas.b.pierron)
(In reply to Abhishek Bhattacharya from comment #54)
> Created attachment 8551259 [details]
> func227-pass17-Sink-mir.gv.png : uceFault_log_number IONGRAPH output
> 
> There you go. Is this the one you need?

Yes, this looks good, this show that the MMathFunction Log is optimized as expected, and on the second attachement, that it is not produced and not optimized.
The following link is a link to the Try server, where we push all the changes before they can be merged into mozilla-central / mozilla-inbound code base.  If everything turns out to be green then this means we are go to go. :)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf0424f7f53c
(In reply to Nicolas B. Pierron [:nbp] from comment #58)
> The following link is a link to the Try server, where we push all the
> changes before they can be merged into mozilla-central / mozilla-inbound
> code base.  If everything turns out to be green then this means we are go to
> go. :)
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf0424f7f53c

All green. ***thumbs_up***
Will you now land the patch for review by the fx-team?
(In reply to Abhishek Bhattacharya from comment #59)
> (In reply to Nicolas B. Pierron [:nbp] from comment #58)
> > The following link is a link to the Try server, where we push all the
> > changes before they can be merged into mozilla-central / mozilla-inbound
> > code base.  If everything turns out to be green then this means we are go to
> > go. :)
> > 
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf0424f7f53c
> 
> All green. ***thumbs_up***
> Will you now land the patch for review by the fx-team?

Why should the fx-team review this patch?  No, I will just push it :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/973ede87dcdd

Congratulation \o/
This modification is now in mozilla-inbound, it will soon be available in Firefox Nightly :)

Is there any other bug/feature that you might be interested to continue on?
(In reply to Nicolas B. Pierron [:nbp] from comment #60)
> (In reply to Abhishek Bhattacharya from comment #59)
> Why should the fx-team review this patch?  No, I will just push it :)
Ohh. All the previous bugs assigned to me always went to the fx-team for review, before they got pushed into a permanent changeset. So you have full commit access? WOW :O
> https://hg.mozilla.org/integration/mozilla-inbound/rev/973ede87dcdd
> 
> Congratulation \o/
> This modification is now in mozilla-inbound, it will soon be available in
> Firefox Nightly :)
> 
> Is there any other bug/feature that you might be interested to continue on?
I would be happy to. But I'm assigned to 3 bugs right now.(2 after this one's RESOLVED FIXED) I plan to finish them (at least one) by this week or so. If any of your mentored bugs are still open by then, I'll be happy to help. Feel free to contact me. (y)
(In reply to Abhishek Bhattacharya from comment #61)
> Ohh. All the previous bugs assigned to me always went to the fx-team for
> review, before they got pushed into a permanent changeset.

I think you might be confusing some terminology. fx-team is a tree (a repository) that some components use to test changes before they're merged into the main tree, mozilla-central. mozilla-inbound works the same way. But neither of them involve a person looking over and critiquing your code (review) - they're just safeguards to avoid breaking mozilla-central too often.

But yes, Nicolas has level 3 commit access [1], so he can push directly to inbound :)

[1] https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/
(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #62)
> (In reply to Abhishek Bhattacharya from comment #61)
> > Ohh. All the previous bugs assigned to me always went to the fx-team for
> > review, before they got pushed into a permanent changeset.
> 
> I think you might be confusing some terminology. fx-team is a tree (a
> repository) that some components use to test changes before they're merged
> into the main tree, mozilla-central. mozilla-inbound works the same way. But
> neither of them involve a person looking over and critiquing your code
> (review) - they're just safeguards to avoid breaking mozilla-central too
> often.
Why use a misleading name? Plus, I've heard of using mozilla-inboud to land patches, nut never the fx-team. Go figure. Thanks Emanuel.
> 
> But yes, Nicolas has level 3 commit access [1], so he can push directly to
> inbound :)
> 
> [1]
> https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/
https://hg.mozilla.org/mozilla-central/rev/68fee019fc00
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: