-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add d8 for disassembling javascript #4721
Add d8 for disassembling javascript #4721
Conversation
98d476e
to
efccae0
Compare
@mattgodbolt Can I get some pointers on how to get a Also, I have just built v8 from its main branch. But I could build from one of the release branches that have semver numbers. However, I don't think there is any place where once can just download a built So, how do I go about putting the binary on the server? |
If there's no prebuilt's available, we usually create our own builder. (https://github.com/compiler-explorer?q=-builder&type=all&language=&sort=) using Docker and some scripts. We usually plop non-massive compiler builds here https://github.com/compiler-explorer/misc-builder/tree/main/build - but that repo is a tiny bit polluted at the moment. But if you can make a build script in the style of those, that would be good. After that, we can invoke that builder and the tarxz will be placed on our S3 ready to download with a tiny configuration change to https://github.com/compiler-explorer/infra The build script is the important part though. |
@gautam1168 thanks for this! @partouf has pointed you in the right direction: how would you best like to do this? If you have a simple bash script that can build |
I am familiar with docker and I can prepare a script for the same. However, I may have to wait till friday before I can commit serious time on this. I also saw a suggestion in the main issue from @jakobkummerow that nodejs can be used instead of a d8 binary. However, with that approach I think it would not be possible to use the --allow-native-syntax flag to cause optimization of functions early as he showed. I will both try out the syntax he showed in the issue and get the docker script ready in this weekend and update my PRs |
Fantastic: Thanks @gautam1168. Our |
@mattgodbolt @partouf I actually just found a DockerFile that builds v8. So my work would be pretty easy if I just wanted to build it like the example above. I could just fork and modify it. However as @jakobkummerow pointed out later, I don't actually have to build Doing this will also make sure that updating to latest stable release of If you think we should do a full build instead I can make a |
I checked the repo that builds python and I see that you guys are building python from source as well. To stay in line with that Ill have to build nodejs from source, so it would be better to build d8. Also, with d8 I will also get the tools necessary to visualize profile data. So I setup a docker repo to build d8. Its building right now in the container. I will update once I have it working just like the other builders. Im not sure how Ill get the repo added to compiler-explorer. But first Ill finish doing the build inside the container. |
ok I have a repository ready that makes a docker image similar to the other builder repositories in compiler-explorer that builds This terminal is in my docker container. So, I can generate the builds as |
Thanks @gautam1168 ! I'll fork your repo into our org and make any changes needed! |
On second thoughts I'll use your repo as inspiration: the dockerfile won't work for us as it stands but is a very useful starting point! thank you :) |
So I did a bunch of work to try and make a reproducible build but it seems the tools really don't like it :D I'm going to take a look another day, I think. |
Oh tell a lie! Your repo is great, I had misread it quite badly...I'll see if I can get this sorted this weekend |
building now! https://github.com/compiler-explorer/d8-builder/actions/runs/4394733377/jobs/7695947983 - forked from your repo (thanks!) and then tweaked. Your thoughts are welcome: I tried to remove things I found unnecessary but I might be deluding myself. |
A trunk build: https://github.com/compiler-explorer/infra/actions/runs/4394838600 - we will need to do this daily for "trunk" with a separate PR. Also - what named versions should I build? |
Hello,
Hey sorry I have not been active in the past 2 weeks. I was in the middle
of leaving my current job so I had a lot of things to do there. I will be
more active from tomorrow.
Thankyou for doing this. I will go through this and respond and see if I
can help in some way.
Regards
Gaurav Gautam
…On Sun, Mar 12, 2023 at 5:49 AM Matt Godbolt ***@***.***> wrote:
A trunk build:
https://github.com/compiler-explorer/infra/actions/runs/4394838600 - we
will need to do this daily for "trunk" with a separate PR. Also - what
named versions should I build?
—
Reply to this email directly, view it on GitHub
<#4721 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACIPIXEERQ5Z5BZ6QRET76LW3UJALANCNFSM6AAAAAAUYUYBFY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@gautam1168 no worries at all! Nothing here is ever time-critical :) I'm just trying to make sure I'm not blocking anything... |
And good luck in whatever you're doing next in your job! |
@mattgodbolt I have made a PR to the d8-builder repository you setup. Mainly to answer the question about which version to build. Please look at my comment there. There are actually two possible stable versions to choose from at any given time and there are two branches for these versions. I am not sure which one ce should go with. Although I would choose the |
Probably it's best to follow Chrome stable releases, e.g.: New releases usually happen every four weeks, you can see the calendar here. The pattern "V8 branch name == Chrome version divided by 10" has been consistently maintained since Chrome 41 IIRC and there are currently no plans to change it, but of course I can't promise that for all eternity. I'd like to preemptively point out that there is no meaning to "major versions" in V8. For example, the step from 10.9 to 11.0 was based on just the same regular 4-week release cadence as 11.0 to 11.1. |
Thankyou @jakobkummerow for clarifying this. I suppose we could just list the branches using |
efccae0
to
58d832c
Compare
@mattgodbolt I have added a I looked at the other compilers in CE and they usually have few versions that are specified in the properties files in the In any case, is there a way I can setup my local environment to check if my configuration for amazon.properties is correct? Or is there an alpha environment where I could test it? I will make a separate pull request to parse the assembly generated by d8 into something that is closer to the output generated by the clang/gcc compilers where you can click to see documentation of instructions and jump to addresses etc. For now I would like to get this change merged with just the default output of v8. I am working some more on this, before I will remove the [Work In Progress] today, however if you can help me with testing the javascript.amazon.properties file it will clear a major blindspot for me. |
actually @mattgodbolt I think it doesn't really make sense to have different versions of v8 listed, because really unlike a llvm based C compiler where people are looking to compare what has changed in the generated assembly from version to version, this one is just there for looking at what is getting generated for a snippet. I personally do not know of anyone who would want the different versions. Although there might be some interest in looking at the assembly generated by other javascript engines for the same code. So I think all we need is the latest version of v8 build. Unless @jakobkummerow tells us otherwise. My own interest in this is to use it in an online course I'm doing to compare the assembly generated by v8 vs the assembly generated by gcc for c++. So I will just wait for your comment now @mattgodbolt to proceed from here. I have removed the [Work In Progress] from this PR and I will make any changes that might be requested to make this PR mergeable. |
@mattgodbolt whenever you have some time can you take a look at this and let me know what I need to do to get this merged? |
I'm so sorry this has taken so long to get feedback to you: I'll do my best to help this over the finish line in the next few days! |
I've added a nightly trunk build installer. It will build d8 nightly from head and install it to |
fb56c0b
to
629e578
Compare
I have added |
629e578
to
5512248
Compare
fac61f2
to
987b6be
Compare
also @mattgodbolt the
Should I add this change to the |
Thanks for spotting that. If you'd like to fix it, please do it in a separate PR! Thanks :) I tend to use an IDE so didn't spot this 😊 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I'll build and install the 11.8 node js and then test locally before a final merge. Thanks so much for your patience.
@@ -0,0 +1,13 @@ | |||
function square(a) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks : I understand what happened here. This workaround works for me!
options= | ||
supportsExecute=false | ||
stubText= | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some extra whitespace here (I can remove it though)
compiler.v8trunk.exe=/opt/compiler-explorer/d8-trunk/d8 | ||
compiler.v8trunk.options=--print-opt-code --redirect-code-traces --allow-natives-syntax | ||
|
||
compiler.v8113.exe=/opt/compiler-explorer/d8-11.3/d8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I'll get this one installed too.
In local testing I can't for the life of me find the actual disassembly output. For example I see:
there's no indication of any |
A blind guess, without having investigated: try some more (e.g. a hundred) invocations of the function before optimizing it. Calling it just once or twice won't be enough to collect feedback, because even feedback collection takes some time to kick in (to save time and memory for code that's only executed once). |
@jakobkummerow the example code has some intrinsic magic thing to alledgedly "optimize on the next call" |
I'll get this deployed and then we can see though |
@mattgodbolt Yes, I was guessing you've been using example code similar to the
Of course you can also get it deployed first and then iterate. |
We should probably update the example code if that's needed :) |
This is now live, and indeed calling it in a loop generates slightly more understandable code! |
Don't have any feedback for the PR but just saw this in my emails and wanted to say I am so excited to see this land! |
Hello. So happy to see this is live now. @mattgodbolt so I think the reason you cannot see the exact multiply instruction is because that is done using a builtin function in the javascript engine instead of a direct assembly instruction. I think its this line over here Although I may be wrong. I have also got the output for the same code in turbolizer and we can see that the bytecode has the multiplication in it: But then it doesn't look like it gets translated to a simple one instruction multiplication even after optimization. It seems to stay as a function call. Also I see that in the live version the trunk compiler is not working. Ill check it today. |
It's cool to see this in action :)
...there's no type feedback, because type feedback collection only starts after a while, and optimized compilation was manually forced before that had happened. So the optimizing compiler doesn't know that it should expect integer multiplications, so it emits code that will just get deoptimized when it's called. The Turbolizer output makes this clear (if you know how to read it...). One workaround is to call the function more often before optimizing it. In this particular example, 10 calls seems to be the threshold, but it would probably be unwise to rely so tightly on engine-internal heuristics that can and do change over time, so 100 calls would be a more robust default choice. Another workaround is to manually trigger the start of feedback collection:
A third workaround, and IMHO the best option, is to add |
Taking a step back, I'm wondering whether it would be (1) feasible and (2) desirable to add a |
Have y’all considered using |
For this issue:
#264