Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

deps: v8 to 3.29.91 #8134

Closed
wants to merge 6 commits into from
Closed

deps: v8 to 3.29.91 #8134

wants to merge 6 commits into from

Conversation

refack
Copy link
Contributor

@refack refack commented Aug 11, 2014

As per Yang Guo @ May 13 v8 removed DebbugerAgent, and we need to evaluate the consequances (might need to reimplement in node-land).
Also no one replied to said message with complaints, only ad-hoc on the review thread.

In return we get "fat-arrow" => lambdas, and modules

@refack refack changed the title deps: update v8 to ^3.28.68 deps: update v8 to ^3.28.68 - WIP Aug 11, 2014
@refack
Copy link
Contributor Author

refack commented Aug 11, 2014

@tjfontaine can someone label this as "in progress" and "v8"?

@tjfontaine
Copy link

@refack added

@refack
Copy link
Contributor Author

refack commented Aug 11, 2014

I just lov me some shiny new syntax
fatarrowftw

@refack refack changed the title deps: update v8 to ^3.28.68 - WIP deps: update v8 to ^3.28.66 - WIP Aug 11, 2014
@refack
Copy link
Contributor Author

refack commented Aug 14, 2014

Can anyone recommend best practice implementation TCP/HTTP server to replace the DebugAgent? @trevnorris ? @bnoordhuis ?

@indutny
Copy link
Member

indutny commented Aug 14, 2014

We have a HTTP server in node, right? :)

@bnoordhuis
Copy link
Member

@refack You can find my work in progress here. Feel free to run with it, I don't have time to work on it right now.

Apropos Fedor's comment, there are a number of issues to consider:

  1. The V8 debug protocol is not really HTTP (but it might be close enough to fool http_parser.)
  2. The debug agent needs to have its own event loop and run in a separate thread (and isolate, if parts are JS) if you want busy-looping JS code to be interruptible. You won't get socket events when the agent runs on the main thread because a busy loop starves the event loop.
  3. The fact that any debug agent JS code has to run in a separate isolate means that you can't use the built-in http module (or pretty much anything that's in lib/.)

@bnoordhuis
Copy link
Member

Oh, another thing: the future of V8's debugger protocol is somewhat uncertain. Chrome doesn't use it and neither does anything else besides node.js (that I know of) so it might get scrapped at some point. @bmeck possibly knows more about that.

@refack
Copy link
Contributor Author

refack commented Aug 14, 2014

@bnoordhuis I figured as much about not being able to dogfood the node HTTP server from /lib.

@indutny
Copy link
Member

indutny commented Aug 14, 2014

Why not, we should support multi-context thing, right? Why can't we just spawn another thread and CreateEnvironment() in it?

@bnoordhuis
Copy link
Member

Yeah, but multi-context is not the same thing as multi-thread. Are you confident that node.js is thread-safe enough to make that work? Because I'm not.

@bmeck
Copy link
Member

bmeck commented Aug 14, 2014

I am currently in discussions about the future of the protocol. Chrome does not use the old debugger protocol but directly interfaces with debug-debugger.js .

For now we can use SendCommand and SetMessageHandler in v8-debug.h , spinning up a new thread is ideal. I have toyed with what @indutny says and it works enough but these are close to hello world examples in complexity.

I would not change/update the protocol until discussions with chrome and ff devtools people complete. There are a lot of discussions going on in emails around that debugger review, feel free to move this discussion there

@refack
Copy link
Contributor Author

refack commented Aug 28, 2014

IT WORKS!
Just 10 failing tests ;)
Temporary fix, I restored DebuggerAgent to v8.
@tjfontaine @indutny @bnoordhuis @bmeck please review

@refack refack changed the title deps: update v8 to ^3.28.66 - WIP deps: update v8 to ^3.29.25 - WIP Aug 28, 2014
@refack
Copy link
Contributor Author

refack commented Aug 28, 2014

ES6 iteration
iteration_over_generator

@bnoordhuis
Copy link
Member

Not sure if I would recommend upgrading to 3.29.x, that's the current development branch.

Did you happen to run the benchmarks? I'm curious if TurboFan in its current state has a positive or negative effect.

@refack
Copy link
Contributor Author

refack commented Aug 28, 2014

I see this whole PR as work for 0.13, so let's go crazy.
Hadn't had the chance to Benchmark, I'll give it a go.
Still have a few tests to resolve, and decouple the DebugAgent from v8.

@YurySolovyov
Copy link

@bnoordhuis isn't TF still in development and disabled by default?

@refack
Copy link
Contributor Author

refack commented Aug 29, 2014

Quick results

benchmarks/arrays: 0.11.14 (0.12) vs. this PR (a.k.a. 0.13.0)

bench1
but the total time for 0.11.14 was real 0m20.093s while for 0.13.0 real 0m12.854s, so it seems like memory allocation is much faster

@bnoordhuis
Copy link
Member

isn't TF still in development and disabled by default?

In development: yes. Disabled: no, except on win64. Which raises the question... @refack is that a 32 or 64 bits executable?

@refack
Copy link
Contributor Author

refack commented Aug 29, 2014

Yea I just sew

#if V8_TURBOFAN_BACKEND && !(V8_OS_WIN && V8_TARGET_ARCH_X64)
#define V8_TURBOFAN_TARGET 1
#else
#define V8_TURBOFAN_TARGET 0
#endif

I compiled for win64. So we only got the backend.

Doing it again for win32, and linux64

@bnoordhuis
Copy link
Member

Don't go to too much trouble; it's not hugely important, I'm mostly just curious.

@refack
Copy link
Contributor Author

refack commented Aug 29, 2014

just recompiling, I'm just as curious as you are.

@refack refack changed the title deps: update v8 to ^3.29.25 - WIP deps: update v8 to ^3.29.33 - WIP Aug 31, 2014
@bnoordhuis
Copy link
Member

Wow, some of the numbers are pretty insane. Thanks for posting that.

@Fishrock123
Copy link
Member

Oh, I just realized that 3.29 is the latest v8 version.

Keep in mind that it will probably not be by the time everything else is fixed up for 0.12 release.

@indutny
Copy link
Member

indutny commented Sep 29, 2014

@refack just a heads up for you. I'm going to rewrite debugger agent in libuv+http-parser, should be quite easy to do.

@refack
Copy link
Contributor Author

refack commented Sep 29, 2014

Go for it. the API is very simple

@indutny
Copy link
Member

indutny commented Sep 29, 2014

Yes, this is what motivates me :) Sorry for redoing your work.

@jonathanong
Copy link

fwiw i'd really like v8 v3.29.70+ to get into node v0.12 because generators are unflagged

@refack
Copy link
Contributor Author

refack commented Sep 29, 2014

@indutny use https://github.com/TheNodeILs/node/compare/v8-next-2, the agent is in a seperate target
You know what. Since I integrated all your comment I'm pushing it here.

@refack
Copy link
Contributor Author

refack commented Sep 29, 2014

@indutny do you have energy to do your magic on this batch?

@indutny
Copy link
Member

indutny commented Sep 30, 2014

Hopefully, yes.

@refack
Copy link
Contributor Author

refack commented Sep 30, 2014

I meant you hawk-eye review ;)

@YurySolovyov
Copy link

Looks like 3.30 is out:
v8/v8@0191cfb
Just FYI

@indutny
Copy link
Member

indutny commented Sep 30, 2014

@YuriSolovyov ok, this means that we could probably try sticking to 3.29 then.

Anyway, here is my progress on this so far: indutny/node@joyent:v0.12...feature/update-v8

Refael Ackermann and others added 6 commits September 30, 2014 12:42
Signed-off-by: Refael Ackermann <refack@gmail.com>
this is in order to give a better development experience in MSVS
eventually should land in v8 trunk
- Getting it to compile and be sane
- Removed references to v8::Debug*
- activated "treat warnings as errors"
- based on discarded v8 code
@indutny
Copy link
Member

indutny commented Sep 30, 2014

Closed in a favor of #8476

@vkurchatkin
Copy link

having hard time compiling this on OS X (clang-600.0.51). Get error: expected a class or namespace on FunctionKind enum. Any ideas?

@refack
Copy link
Contributor Author

refack commented Sep 30, 2014

Why closed it it's targeted at master?

@refack
Copy link
Contributor Author

refack commented Sep 30, 2014

@vkurchatkin you need to enable c++11

@indutny
Copy link
Member

indutny commented Oct 1, 2014

@refack there is no point in targeting master right now, we'll do a merge after releasing v0.12. @vkurchatkin are you using my branch?

@vkurchatkin
Copy link

@refack I figured that much, but shouldn't that be set somewhere in gyp configs?

@indutny nope, this one. FunctionKind seems to be a new thing.

@refack
Copy link
Contributor Author

refack commented Oct 1, 2014

@vkurchatkin Yes. I covered it for linux and windows. On it.

@refack
Copy link
Contributor Author

refack commented Oct 1, 2014

@vkurchatkin try my v8-next branch now - https://github.com/TheNodeILs/node/tree/v8-next

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

Successfully merging this pull request may close these issues.

None yet