Skip to content
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

Option to run V8 test suite #199

Closed
rvagg opened this issue Sep 22, 2015 · 38 comments
Closed

Option to run V8 test suite #199

rvagg opened this issue Sep 22, 2015 · 38 comments
Assignees

Comments

@rvagg
Copy link
Member

rvagg commented Sep 22, 2015

Requested by @bnoordhuis @ nodejs/node#2999 (comment)

Sounds like a good idea to me. Do we have enough in deps/v8 to run the tests in-tree without additional tools or do things get complicated when you need to do this?

@bnoordhuis
Copy link
Member

Yes and no. The source tree in deps/v8 contains the test runner but there are build dependencies that must be checked out with gclient sync from depot_tools (making depot_tools a dependency as well.)

@rvagg
Copy link
Member Author

rvagg commented Sep 22, 2015

a mention of depot_tools makes my eyes glaze over .. sounds like it's going to be complicated and very slow

@bnoordhuis
Copy link
Member

Complicated maybe, but it doesn't have to be slow. gclient sync is basically git fetch plus some post-checkout actions. To keep checkout times down, keep a pristine source tree around, sync it, then cp -a it somewhere and build and test that.

@jbergstroem
Copy link
Member

I don't think we can use v8 from deps/v8; gclient expects a proper git tree. Running this on all slaves sounds like a bit of a stretch, but even getting one vm testing (debian 8, freebsd or smartos vm's has most cpu to spare) would be pretty great.

Edit: I guess we'd have to check out the tag and then reapply our floating patches to properly test it.

@mhdawson
Copy link
Member

+1 on the idea for me. I think @jasnell had done some initial work looking at doing this.

@jasnell
Copy link
Member

jasnell commented Sep 22, 2015

There's a bit of work involved but I had this working in joyent/node master. I can update and get it running again. It'll just take a couple weeks given current workload

@pmq20
Copy link

pmq20 commented Sep 23, 2015

If we were to land fixes about v8 this definitely is a must.

@rvagg
Copy link
Member Author

rvagg commented Sep 23, 2015

right, particularly as we move into LTS-land where we're supporting old V8

@joransiu
Copy link

Pinged @jasnell yesterday, and he pointed us to his old PR nodejs/node-v0.x-archive#14185 on this topic. We're looking into reviving and flushing out some of his changes there.

/cc @exinfinitum

@exinfinitum
Copy link

@jasnell I found a way to run v8 tests in node. This was originally developed for zLinux version of node, to run tests on v8z (a port of v8 to IBM s390(x) CPUs), but should also work for v8. Checkout https://github.com/exinfinitum/node/tree/al-merge (branch "al-merge") and run the following commands:

make v8 DESTCPU=(ARCH)
make test-v8 DESTCPU=(ARCH)

where (ARCH) is your CPU architecture, e.g. x64, ia32. DESTCPU MUST be specified for this to work properly.

Can also do tests on debug build by using "make test-v8 DESTCPU=(ARCH) BUILDTYPE=Debug".

@mhdawson
Copy link
Member

If its not too much work could you create a PR that applies to master so we could try it out there. I think it would also trim down what needs to be reviewed to the minimal set. James is on holiday this week so probably worth doing this instead of waiting to hear from him.

@mhdawson
Copy link
Member

PR here nodejs/node#4704

@jbergstroem
Copy link
Member

Do we have other constraints? For instance -- do we need more than 2G memory? Is the idea to run this on all slaves or a subset?

@mhdawson
Copy link
Member

At least for now the patch does not support windows but other than that I think we do want to run across all platforms as there is platform specific code and so we might have floating patches that are platform specific

I'm not sure if it needs more than 2G, I know that all the machines we currently run on have more than that. I'm away next week but can figure that out when I'm back.

@jbergstroem
Copy link
Member

Actually, most machines we deploy today run on 2G. We have alternate deployment for specific needs though which could also cater v8. I just need a better understanding on what's required. Also, how long would a run take?

@mhdawson
Copy link
Member

The actual run of the tests on an x64 box took 4 1/2 minutes on my x64 machine, the build was more in the 10-15 minute timeframe but that's without cc cache enabled so I'd like to see how long it takes on one of the CI machines.

If I could install svn on one of the x64 test machines I can get more specific numbers.

@jbergstroem
Copy link
Member

Sounds good to me. Test away.

@mhdawson
Copy link
Member

mhdawson commented Feb 2, 2016

Have it working on x86 on a machine with 2 cpus/4G. Took 33mins. I think we could make this shorter if we ran with more cpus.

https://ci.nodejs.org/job/mdawson-RunV8TestsInNode/

@mhdawson
Copy link
Member

@jbergstroem
Copy link
Member

Thinking the crash is a memory issue:

=== mjsunit/regress/regress-crbug-514081 ===
Command: /home/iojs/build/workspace/node-test-commit-v8-linux-mdawson/nodes/test-softlayer-ubuntu14-x64-1/deps/v8/out/x64.release/d8 --test --random-seed=910294543 --stress-opt --always-opt --nohard-abort --nodead-code-elimination --nofold-constants /home/iojs/build/workspace/node-test-commit-v8-linux-mdawson/nodes/test-softlayer-ubuntu14-x64-1/deps/v8/test/mjsunit/mjsunit.js /home/iojs/build/workspace/node-test-commit-v8-linux-mdawson/nodes/test-softlayer-ubuntu14-x64-1/deps/v8/test/mjsunit/regress/regress-crbug-514081.js
exit code: -9
--- CRASHED ---

@mhdawson
Copy link
Member

It crashed with 4G as well but I did have the same thought. Will do a run on a machine with 8G to see.

Separately machines with svn now
test-softlayer-ubuntu14-x64-1
test-digitalocean-centos7-x64-1
iojs-ibm-ppcle-ubuntu1404-64-2

@mhdawson
Copy link
Member

PR to start the additional of svn #330

@mhdawson
Copy link
Member

add svn to iojs-softlayer-benchmark as it has 8G memory and is ubuntu 14

@mhdawson
Copy link
Member

Build here to compare across 2, 4 and 8 G
https://ci.nodejs.org/job/node-test-commit-v8-linux-mdawson/

@mhdawson
Copy link
Member

Build above shows that the crash failure does not occur on machines with 8G but that there is a new failure on the 2 ubuntu machines with 8G

nodejs/node#5263

@mhdawson
Copy link
Member

Added svn to test-digitalocean-freebsd10-x64-1

@mhdawson
Copy link
Member

Added svn to test-rackspace-debian8-x64-1

@mhdawson
Copy link
Member

Added svn to test-joyent-smartos14-x64-1

mhdawson added a commit that referenced this issue Mar 24, 2016
Added svn as module to be installed in order
to support #199

Did not add for raspberry pi as I don't think they
are powerful enough to run the v8 tests

Fixes: #306
PR-URL: #33
Reviewed-By: Johan Bergström <[email protected]>
@mhdawson
Copy link
Member

Ok, back to looking a this. The last issue I was looking at was a failure on arm. Now though, after a few runs the runs under the CI to check current state the runs across the board, on arm are not even getting through the compilation step.

The runs take a long time (2.5 hours+) the machines go offline before the compile actually finishes. This was also occurring when I was trying to debug locally.

At this point I think the right answer is not to block setting up/running for arm and create a new issue to cover future work to add support for windows, mac, arm, smartos that interested people can pick up. In some cases like mac and arm adding them may just require enough hw while for windows work will be required on the build files.

Current coverage is ubuntu 14 for ppcbe, ppcle, and x64 and I'll see if our smartos machines are big enough to run/pass the tests in a reasonable time. I'll then enable the nightly job and open a new issue as a placeholder to remind us we may want to add the other platforms at some point when h/w time permits

@mhdawson
Copy link
Member

Tried smartos but the machines seems to choke with a bunch of :"virtual memory exhausted: Resource temporarily unavailable" messages. I'm also thinking that since smartos is x86 we already have it covered by the unbuntu x86 case so will leave it out for now.

@mhdawson
Copy link
Member

Issue for additional coverage - #387

Job configured and scheduled to run nightly here - https://ci.nodejs.org/job/node-test-commit-v8-linux/

@mhdawson
Copy link
Member

@nodejs/collaborators FYI there is a new job which runs the v8 tests after building v8 from the source in the node tree. This should be used when floating patches on top of v8 in addition to the regular CI runs. It is also scheduled to run once a day. It currently only covers ubuntu for x64, ppc le and ppc be, see #387 if you are interested in expanding this coverage.

@misterdjules
Copy link
Contributor

@mhdawson

I'm also thinking that since smartos is x86 we already have it covered by the unbuntu x86 case so will leave it out for now.

Did you mean that the JIT compiler's behavior is essentially the same between Ubuntu x86 and SmartOS because it's the same architecture, and so testing on Ubuntu x86 is enough to test SmartOS? I would think that these tests also test other parts of V8 that are dependent on specifics of the underlying OS, and so assuming that the SmartOS platform is supported if Ubuntu x86 tests pass does not seem like a reasonable assumption.

I think we agree on that but I just want to make sure I'm not missing anything.

Thank you!

@jbergstroem
Copy link
Member

I don't think ubuntu x86 would cover SmartOS x86. These machines has 4G ram which might explain why you're running out of memory. I can reprovision one of the x86's and one of the x64's to have 8G instead?

@bnoordhuis
Copy link
Member

Did you mean that the JIT compiler's behavior is essentially the same between Ubuntu x86 and SmartOS because it's the same architecture, and so testing on Ubuntu x86 is enough to test SmartOS? I would think that these tests also test other parts of V8 that are dependent on specifics of the underlying OS, and so assuming that the SmartOS platform is supported if Ubuntu x86 tests pass does not seem like a reasonable assumption.

Codegen is the same for smartos and linux, so in that respect they're interchangeable. Also, it's not as if upstream V8 supports smartos in any capacity.

@mhdawson
Copy link
Member

Ideally we'd run everywhere, but my comment on smartos was that at least the arch is covered (x86) and as Ben mentions codegen is the same. I agree that does not provided full coverage and am supportive of adding to the test as covered by #387. By the same token I've not added AIX yet but at least we have PPC coverage which is the same arch.

@misterdjules
Copy link
Contributor

@bnoordhuis @bnoordhuis I'm aware that codegen is the same on SmartOS x86 and Linux x86 , I was specifically mentioning parts of V8 that depend on the underlying OS, like code in v8/src/base/platform. It seems we're on the same page though that this is good enough but not ideal. Thanks for the clarification!

@mhdawson
Copy link
Member

scheduled job ran/passed last night so going to close this one and discussion on adding other platforms can continue under #387. Going to close this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants