[v8-dev] Re: Status of LoopPeeling

[jarin via v8-dev]

On Sunday, September 9, 2018 at 4:50:38 AM UTC+2, Bekket McClane wrote:
>
> Hi,
>
> I'm currently studying some graph reduction passes.
> It seems that LoopPeeling is only used when certain flag is turn on. I'm 
> curious about the status and usage of this pass
>

The pass is on by default.
 

>
> Also, though the algorithm does the eligibility check, it doesn't do any 
> suitability check - it just peels the first iteration anyway. To me, this 
> doesn't make much sense, since force peeling the first iteration not always 
> give you performance improvement. Is there any specific reasons for doing 
> this?
>

Right, we just have not done the work of figuring out a suitable heuristic 
to enable loop peeling. We had some rough ideas, but we have never tried 
them. Do you know of any good heuristics for loop peeling (or literature 
where it is discussed)?

Cheers, Jaro
 

>
> Thank you.
>
> Bekket
>

-- 
-- 
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups 
"v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [v8-dev] A potential speedup for Promises and Async Functions

[jarin via v8-dev]

On Monday, September 4, 2017 at 1:43:11 PM UTC+2, Caitlin Potter wrote:
>
>
> On Sep 4, 2017, at 4:08 AM, jarin via v8-dev <v8-...@googlegroups.com 
> > wrote:
>
>
>
> On Friday, September 1, 2017 at 6:29:50 PM UTC+2, Jakob Kummerow wrote:
>>
>> As Yang points out, microtasks are only run when the embedder asks us to, 
>> and per spec this is only when the execution stack is otherwise empty. (The 
>> exception to this is %RunMicroTasks() in mjsunit tests.)
>> So we only really care about a C++ entry point.
>>
>> However, what RunMicrotasks does is loop over the queue and call into JS 
>> for every iteration, because the entries in the queue are JS callables. 
>> AFAIK going through the JSEntryStub has even more overhead than going 
>> through the CEntryStub, so the idea here is to cross the language boundary 
>> only once (into a CSA builtin), and then loop there. This is the mirror 
>> image of another trick we pull more commonly: converting a JS builtin with 
>> a loop that calls the runtime on every iteration with a single runtime call 
>> that loops in C++.
>>
>> AFAICT the only existing technology we have for calling CSA-generated 
>> code from C++ is by wrapping it into a JSFunction. The JSFunctions where we 
>> do this so far, however, are all tied to a native context; whereas the 
>> RunMicrotasks CSA builtin would be context-independent (just like its 
>> existing C++ equivalent). When Adam and I looked at the source, it seemed 
>> to us that creating a JSFunction with a nullptr context should work (at 
>> least for getting through the JSEntryStub), and would allow us to reuse the 
>> existing mechanism.
>>
>> If that idea doesn't fly, then do we have any options beyond mucking with 
>> the JSEntry stub to make it possible to call CSA builtins directly?
>>
>
> I and Benedikt were actually thinking that mucking with the JSEntry stub 
> would be lesser evil. All we should need to change in JSEntry would be to 
> add a call to a stub that would check whether it needs to process the 
> microtask queue and do it. (I guess this is similar to what Caitlin already 
> did when she said "Finally, I tried adding a new variant to JSEntryStub, 
> which call the RunMicrotasks stub rather than the various entry 
> trampolines."
>
>
> I'm trying to understand...
>
> So instead of "if target === RunMicrotasks root code, call the stub", you 
> want JSEntryStub to check if there are pending microtasks to run? That 
> seems like it would alter the API.
>
> I'm sure I'm just misunderstanding what is meant by "be to add a call to 
> a stub that would check whether it needs to process the microtask queue and 
> do it".
>

Oh, you have not misunderstood, I indeed did not quite know what I was 
talking about. "if target === RunMicrotasks root code, call the stub" makes 
sense now. Let me think about it a bit more now.

 

>
> ) Yes, it does sound a bit scary, but the JSFunction with nullptr context 
> seems scary + hacky.
>
> Eventually, we might consider writing more general machinery for calling 
> arbitrary CSA from C++, but I think we should only do it once there are 
> more customers for this.
>  
>
>>
>>
>> On Fri, Sep 1, 2017 at 6:18 AM, Caitlin Potter <caitpo...@gmail.com> 
>> wrote:
>>
>>>
>>> On Sep 1, 2017, at 2:36 AM, Benedikt Meurer <bme...@chromium.org> wrote:
>>>
>>> I think we're trying to solve an actual problem, plus a problem that we 
>>> might not have. Let's just keep the C++ version of RunMicrotaskQueue for 
>>> now and work on the real issue only. This code doesn't change often and 
>>> there's otherwise no advantage but a lot of potential breakage when messing 
>>> with strong-rooted JSFunctions, JSEntryStub and friends.
>>>
>>>
>>> Which is the "actual" problem, and which is the one we might not have?
>>>
>>> -- Benedikt
>>>
>>> On Fri, Sep 1, 2017 at 8:28 AM Caitlin Potter <ca...@igalia.com> wrote:
>>>
>>>>
>>>> On Sep 1, 2017, at 1:57 AM, Yang Guo <yan...@chromium.org> wrote:
>>>>
>>>> I don't like strong-rooting a JSFunction either. You will get some 
>>>> issues with the serializer when creating the snapshot.
>>>>
>>>>
>>>> Fair enough, but then we do need a way to enter the stub cleanly.
>>>>
>>>> I also feel like I don't understand the problem. We run the 
>>>> MicrotaskQueue when we are about to return to the embedder with 
>>>> 

Re: [v8-dev] A potential speedup for Promises and Async Functions

[jarin via v8-dev]

On Friday, September 1, 2017 at 6:29:50 PM UTC+2, Jakob Kummerow wrote:
>
> As Yang points out, microtasks are only run when the embedder asks us to, 
> and per spec this is only when the execution stack is otherwise empty. (The 
> exception to this is %RunMicroTasks() in mjsunit tests.)
> So we only really care about a C++ entry point.
>
> However, what RunMicrotasks does is loop over the queue and call into JS 
> for every iteration, because the entries in the queue are JS callables. 
> AFAIK going through the JSEntryStub has even more overhead than going 
> through the CEntryStub, so the idea here is to cross the language boundary 
> only once (into a CSA builtin), and then loop there. This is the mirror 
> image of another trick we pull more commonly: converting a JS builtin with 
> a loop that calls the runtime on every iteration with a single runtime call 
> that loops in C++.
>
> AFAICT the only existing technology we have for calling CSA-generated code 
> from C++ is by wrapping it into a JSFunction. The JSFunctions where we do 
> this so far, however, are all tied to a native context; whereas the 
> RunMicrotasks CSA builtin would be context-independent (just like its 
> existing C++ equivalent). When Adam and I looked at the source, it seemed 
> to us that creating a JSFunction with a nullptr context should work (at 
> least for getting through the JSEntryStub), and would allow us to reuse the 
> existing mechanism.
>
> If that idea doesn't fly, then do we have any options beyond mucking with 
> the JSEntry stub to make it possible to call CSA builtins directly?
>

I and Benedikt were actually thinking that mucking with the JSEntry stub 
would be lesser evil. All we should need to change in JSEntry would be to 
add a call to a stub that would check whether it needs to process the 
microtask queue and do it. (I guess this is similar to what Caitlin already 
did when she said "Finally, I tried adding a new variant to JSEntryStub, 
which call the RunMicrotasks stub rather than the various entry 
trampolines.") Yes, it does sound a bit scary, but the JSFunction with 
nullptr context seems scary + hacky.

Eventually, we might consider writing more general machinery for calling 
arbitrary CSA from C++, but I think we should only do it once there are 
more customers for this.
 

>
>
> On Fri, Sep 1, 2017 at 6:18 AM, Caitlin Potter  > wrote:
>
>>
>> On Sep 1, 2017, at 2:36 AM, Benedikt Meurer > > wrote:
>>
>> I think we're trying to solve an actual problem, plus a problem that we 
>> might not have. Let's just keep the C++ version of RunMicrotaskQueue for 
>> now and work on the real issue only. This code doesn't change often and 
>> there's otherwise no advantage but a lot of potential breakage when messing 
>> with strong-rooted JSFunctions, JSEntryStub and friends.
>>
>>
>> Which is the "actual" problem, and which is the one we might not have?
>>
>> -- Benedikt
>>
>> On Fri, Sep 1, 2017 at 8:28 AM Caitlin Potter > > wrote:
>>
>>>
>>> On Sep 1, 2017, at 1:57 AM, Yang Guo  
>>> wrote:
>>>
>>> I don't like strong-rooting a JSFunction either. You will get some 
>>> issues with the serializer when creating the snapshot.
>>>
>>>
>>> Fair enough, but then we do need a way to enter the stub cleanly.
>>>
>>> I also feel like I don't understand the problem. We run the 
>>> MicrotaskQueue when we are about to return to the embedder with re-entrancy 
>>> level of 0 
>>> ,
>>>  
>>> so we already need to leave JS and enter JS once. With the micro-benchmark, 
>>> only one microtask gets queued per microtask run, so you don't benefit from 
>>> staying in JS. Of course you could run the MicrotaskQueue before leaving 
>>> JS, but that doesn't seem like what you are trying to do.
>>>
>>>
>>> Even though only one task is enqueued at a time, RunMicrotasks() never 
>>> finishes until the entire benchmark is run (since each resume enqueues 
>>> another task). Many tasks are run in a single JS entry, whereas in v8 ToT 
>>> you enter/exit JS thousands of times.
>>>
>>> Cheers,
>>>
>>> Yang
>>>
>>> On Fri, Sep 1, 2017 at 7:29 AM Benedikt Meurer >> > wrote:
>>>
 I don't think strong-rooting a JSFunction is a good idea. That might 
 solve one problem, but will likely create N+1 different problems in other 
 places.

 I've been discussing this with +Jaroslav Sevcik  and we 
 probably don't understand the underlying problem. So let's try to get that 
 first:
 You're porting the microtask queue pumping to the CSA now, and you need 
 to call into Blink C++ functions and JSFunctions from that. But there's 
 also still the C++ implementation of RunMicrotaskQueue still. Is that 
 correct?

 -- Benedikt

 On Fri, Sep 1, 2017 at 7:03 AM Caitlin Potter  

[v8-dev] Re: Make all registers addressable in Operands (issue 1287383003 by da...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1287383003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] When typing phi nodes, weaken union of integer constants to a range. (issue 1319703006 by n...@chromium.org)

[jarin]

lgtm with a nit.


https://codereview.chromium.org/1319703006/diff/20001/src/compiler/typer.cc
File src/compiler/typer.cc (right):

https://codereview.chromium.org/1319703006/diff/20001/src/compiler/typer.cc#newcode562
src/compiler/typer.cc:562: Type* myinteger = Type::Intersect(type,
integer, zone());
Could you come up with a better name than myinteger? (E.g.. integer_part
or some such.)

https://codereview.chromium.org/1319703006/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Construct Range rather than Constant when typing integers. (issue 1328193003 by n...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1328193003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [runtime] Replace many buggy uses of %_CallFunction with %_Call. (issue 1325573004 by bmeu...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1325573004/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [calls] Consistent call protocol for calls. (issue 1330033002 by bmeu...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1330033002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [stubs] Unify the various versions of [[Call]] with CallCallableStub. (issue 1311013008 by bmeu...@chromium.org)

[jarin]

looks good so far.


https://codereview.chromium.org/1311013008/diff/80001/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):

https://codereview.chromium.org/1311013008/diff/80001/src/x64/builtins-x64.cc#newcode1732
src/x64/builtins-x64.cc:1732: // TODO(bmeurer): This doesn't match the
ES6 spec for [[Call]] on proxies.
You might want to add @neis to this TODO.

https://codereview.chromium.org/1311013008/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [stubs] Unify the various versions of [[Call]] with CallCallableStub. (issue 1311013008 by bmeu...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1311013008/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [turbofan] Greedy: split around calls heuristic. (issue 1328783002 by mtro...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1328783002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Include individual deferred block ranges in splintering. (issue 1322703003 by mtro...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1322703003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Atomics] Remove support for atomic accesses on floating-point values. (issue 1318713007 by bi...@chromium.org)

[jarin]

lgtm with a nit.


https://codereview.chromium.org/1318713007/diff/20001/src/runtime/runtime-atomics.cc
File src/runtime/runtime-atomics.cc (right):

https://codereview.chromium.org/1318713007/diff/20001/src/runtime/runtime-atomics.cc#newcode490
src/runtime/runtime-atomics.cc:490: case kExternalFloat64Array:
Does it really make sense to mention the float cases explicitly here? If
yes, could you remove the default case?

https://codereview.chromium.org/1318713007/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Remove no-zone versions of intersection and union. (issue 1312893010 by n...@chromium.org)

[jarin]

lgtm, thanks!

https://codereview.chromium.org/1312893010/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [runtime] Introduce a dedicated JSIteratorResult type. (issue 1302173007 by bmeu...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1302173007/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [turbofan] Small fix in live range printer. (issue 1326643006 by mtro...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1326643006/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] [turbofan] Do not force stack slot for eager deopt inputs. (issue 1307203005 by ja...@chromium.org)

[jarin]

Reviewers: Benedikt Meurer,

Message:
Benedikt, could you take a look, please?

Description:
[turbofan] Do not force stack slot for eager deopt inputs.

R=bmeu...@chromium.org

Please review this at https://codereview.chromium.org/1307203005/

Base URL: https://chromium.googlesource.com/v8/v8.git@master

Affected files (+28, -14 lines):
  M src/compiler/instruction-selector.h
  M src/compiler/instruction-selector.cc


Index: src/compiler/instruction-selector.cc
diff --git a/src/compiler/instruction-selector.cc  
b/src/compiler/instruction-selector.cc
index  
0ae1c63fea2a336dc55028d2dfa62267e125490f..03106d69e5b8bf772433d9f9235b0399d1cdc2ab  
100644

--- a/src/compiler/instruction-selector.cc
+++ b/src/compiler/instruction-selector.cc
@@ -360,7 +360,8 @@ void InstructionSelector::InitializeCallBuffer(Node*  
call, CallBuffer* buffer,

 Node* frame_state =
 call->InputAt(static_cast(buffer->descriptor->InputCount()));
 AddFrameStateInputs(frame_state, >instruction_args,
-buffer->frame_state_descriptor);
+buffer->frame_state_descriptor,
+FrameStateInputKind::kStackSlot);
   }
   DCHECK(1 + buffer->frame_state_value_count() ==
  buffer->instruction_args.size());
@@ -1016,7 +1017,7 @@ void InstructionSelector::VisitDeoptimize(Node*  
value) {

   sequence()->AddFrameStateDescriptor(desc);
   args.push_back(g.TempImmediate(state_id.ToInt()));

-  AddFrameStateInputs(value, , desc);
+  AddFrameStateInputs(value, , desc, FrameStateInputKind::kAny);

   DCHECK_EQ(args.size(), arg_count);

@@ -1059,7 +1060,8 @@ FrameStateDescriptor*  
InstructionSelector::GetFrameStateDescriptor(

 }


-static InstructionOperand SlotOrImmediate(OperandGenerator* g, Node*  
input) {

+InstructionOperand InstructionSelector::OperandForDeopt(
+OperandGenerator* g, Node* input, FrameStateInputKind kind) {
   switch (input->opcode()) {
 case IrOpcode::kInt32Constant:
 case IrOpcode::kNumberConstant:
@@ -1067,19 +1069,25 @@ static InstructionOperand  
SlotOrImmediate(OperandGenerator* g, Node* input) {

 case IrOpcode::kHeapConstant:
   return g->UseImmediate(input);
 default:
-  return g->UseUniqueSlot(input);
+  switch (kind) {
+case FrameStateInputKind::kStackSlot:
+  return g->UseUniqueSlot(input);
+case FrameStateInputKind::kAny:
+  return g->Use(input);
+  }
   }
 }


-void InstructionSelector::AddFrameStateInputs(
-Node* state, InstructionOperandVector* inputs,
-FrameStateDescriptor* descriptor) {
+void InstructionSelector::AddFrameStateInputs(Node* state,
+  InstructionOperandVector*  
inputs,
+  FrameStateDescriptor*  
descriptor,

+  FrameStateInputKind kind) {
   DCHECK_EQ(IrOpcode::kFrameState, state->op()->opcode());

   if (descriptor->outer_state()) {
 AddFrameStateInputs(state->InputAt(kFrameStateOuterStateInput), inputs,
-descriptor->outer_state());
+descriptor->outer_state(), kind);
   }

   Node* parameters = state->InputAt(kFrameStateParametersInput);
@@ -1098,23 +1106,23 @@ void InstructionSelector::AddFrameStateInputs(

   OperandGenerator g(this);
   size_t value_index = 0;
-  inputs->push_back(SlotOrImmediate(, function));
+  inputs->push_back(OperandForDeopt(, function, kind));
   descriptor->SetType(value_index++, kMachAnyTagged);
   for (StateValuesAccess::TypedNode input_node :
StateValuesAccess(parameters)) {
-inputs->push_back(SlotOrImmediate(, input_node.node));
+inputs->push_back(OperandForDeopt(, input_node.node, kind));
 descriptor->SetType(value_index++, input_node.type);
   }
   if (descriptor->HasContext()) {
-inputs->push_back(SlotOrImmediate(, context));
+inputs->push_back(OperandForDeopt(, context, kind));
 descriptor->SetType(value_index++, kMachAnyTagged);
   }
   for (StateValuesAccess::TypedNode input_node :  
StateValuesAccess(locals)) {

-inputs->push_back(SlotOrImmediate(, input_node.node));
+inputs->push_back(OperandForDeopt(, input_node.node, kind));
 descriptor->SetType(value_index++, input_node.type);
   }
   for (StateValuesAccess::TypedNode input_node : StateValuesAccess(stack))  
{

-inputs->push_back(SlotOrImmediate(, input_node.node));
+inputs->push_back(OperandForDeopt(, input_node.node, kind));
 descriptor->SetType(value_index++, input_node.type);
   }
   DCHECK(value_index == descriptor->GetSize());
Index: src/compiler/instruction-selector.h
diff --git a/src/compiler/instruction-selector.h  
b/src/compiler/instruction-selector.h
index  
9a0744720a099295361e8ab603bb7f6277d0bfcc..b8354fcfd1e2aa07bcda1337c1c18687fe444a41  
100644

--- a/src/compiler/instruction-selector.h
+++ b/src/compiler/instruction-selector.h
@@ -22,6 +22,7 @@ class BasicBlock;
 struct CallBuffer;  // 

[v8-dev] Re: [es6] Fix invalid ToObject in String/Array iterator next. (issue 1325023003 by bmeu...@chromium.org)

[jarin]

lgtm with a nit.


https://codereview.chromium.org/1325023003/diff/1/src/array-iterator.js
File src/array-iterator.js (right):

https://codereview.chromium.org/1325023003/diff/1/src/array-iterator.js#newcode82
src/array-iterator.js:82: !HAS_DEFINED_PRIVATE(iterator,
arrayIteratorNextIndexSymbol)) {
Technically, you should check for presence of the other internal fields,
but I suppose that if arrayIteratorNextIndexSymbol is there, the others
have to be there as well. Could you add a comment that explains this?

https://codereview.chromium.org/1325023003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Remove harmony-atomics flag and collapse it into sharedarraybuffer flag (issue 1304163010 by bi...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1304163010/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Cleanup representation calculation for Phis in Crankshaft. (issue 1315193010 by ja...@chromium.org)

[jarin]

Reviewers: Benedikt Meurer,

Message:
Could you take a look, please?

Description:
Cleanup representation calculation for Phis in Crankshaft.

This replaces the counters for use representations with
simple tracking of most-general representation seen so far.

R=bmeu...@chromium.org
BUG=

Please review this at https://codereview.chromium.org/1315193010/

Base URL: https://chromium.googlesource.com/v8/v8.git@master

Affected files (+46, -77 lines):
  M src/hydrogen-instructions.h
  M src/hydrogen-instructions.cc


--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Revert "Revert of [turbofan] greedy: heuristic for memory operands (issue 1314423006 by mtro...@chromium.org)

[jarin]

lgtm, although I do not fully understand why there is the call to
FindOptimalSplitPos after we find a valid split position.

https://codereview.chromium.org/1314423006/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Version 4.6.85.9 (cherry-pick) (issue 1326463002 by mvstan...@chromium.org)

[jarin]

On 2015/08/31 06:06:30, mvstanton wrote:

lgtm (rubber-stamp)

https://codereview.chromium.org/1326463002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Crankshaft is now able to compile top level code even if there is a ScriptContext. (issue 1317383002 by ish...@chromium.org)

[jarin]

lgtm with some non-blocking questions.


https://codereview.chromium.org/1317383002/diff/60001/src/full-codegen/arm/full-codegen-arm.cc
File src/full-codegen/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/1317383002/diff/60001/src/full-codegen/arm/full-codegen-arm.cc#newcode247
src/full-codegen/arm/full-codegen-arm.cc:247: function_in_register =
true;
Why is this here? It looks like function_in_register is not used after
this point...

https://codereview.chromium.org/1317383002/diff/60001/src/full-codegen/arm/full-codegen-arm.cc#newcode293
src/full-codegen/arm/full-codegen-arm.cc:293: __ mov(r1,
Operand(Smi::FromInt(rest_index)));
I do not quite understand why this cannot overwrite the function in r1,
but I see that it was similar situation in the original code. Are you
sure this is ok? (Same thing above.)

https://codereview.chromium.org/1317383002/diff/60001/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/1317383002/diff/60001/src/hydrogen.h#newcode2034
src/hydrogen.h:2034: return HParameter::New(isolate(), zone(), nullptr,
index);
Do I understand correctly that you had to add these because you want to
pass nullptr as context?

https://codereview.chromium.org/1317383002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [turbofan] Re-wire greedy. (issue 1320363002 by mtro...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1320363002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [Atomics] Remove support for atomic accesses on floating-point values. (issue 1318713007 by bi...@chromium.org)

[jarin]

On 2015/08/31 22:09:51, binji wrote:

The latest spec no longer supports atomic floats.


It looks like there have been something strange going on with
CompareExchangeSeqCst on Windows.

https://codereview.chromium.org/1318713007/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Crankshaft is now able to compile top level code even if there is a ScriptContext. (issue 1317383002 by ish...@chromium.org)

[jarin]

lgtm.

https://codereview.chromium.org/1317383002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [turbofan] Live Range unit tests (issue 1315113003 by mtro...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1315113003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Signal a blocked futex if the isolate is interrupted; don't busy-wait (issue 1230303005 by bi...@chromium.org)

[jarin]

On 2015/08/20 23:16:13, binji wrote:

On 2015/08/20 at 05:19:58, jarin wrote:
 On 2015/08/19 17:36:23, binji wrote:
   
https://codereview.chromium.org/1230303005/diff/120001/src/execution.cc

  File src/execution.cc (right):
 
 


https://codereview.chromium.org/1230303005/diff/120001/src/execution.cc#newcode418
  src/execution.cc:418: if  
(isolate_-futex_wait_list_node()-waiting()) {

  On 2015/08/19 at 05:26:05, Jarin wrote:
   Could we perhaps pull the waiting() check into NotifyWake?
  
   Then the waiting accessors could be private (and perhaps we could  
even

pull
  the waiting_ flag into the critical section and make it non-atomic).
 
  My thought here was to do a quick check to avoid taking a lock if  
there is

no
  futex waiting (which is the common case). What do you think?

 Yeah, that's what I thought. However, locks are cheap, interrupts are  
rare

and

atomics are scary. Up to you.



Ah, good call. I was mistakenly thinking this was a lock grabbed in
HandleInterrupts, not RequestInterrupt.


Thanks! lgtm.

Let's see what the fuzzer thinks :-)

https://codereview.chromium.org/1230303005/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [simd] Introduce SIMD types (as classes) (issue 1303863002 by rossb...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1303863002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] [turbofan] Do not optimize with-context allocation in global code. (issue 1292383007 by ja...@chromium.org)

[jarin]

Reviewers: Michael Starzinger,

Message:
Could you take a look, please?

Description:
[turbofan] Do not optimize with-context allocation in global code.

BUG=
R=mstarzin...@chromium.org

Please review this at https://codereview.chromium.org/1292383007/

Base URL: https://chromium.googlesource.com/v8/v8.git@master

Affected files (+6, -2 lines):
  M src/compiler/js-typed-lowering.cc


Index: src/compiler/js-typed-lowering.cc
diff --git a/src/compiler/js-typed-lowering.cc  
b/src/compiler/js-typed-lowering.cc
index  
628e7e07b69b1ae2ec4560e6c41058d2abdd58c5..68c3e308dc7df9c26ab9f92d5883389fce4c67d8  
100644

--- a/src/compiler/js-typed-lowering.cc
+++ b/src/compiler/js-typed-lowering.cc
@@ -1189,12 +1189,16 @@ Reduction  
JSTypedLowering::ReduceJSCreateLiteralObject(Node* node) {

 Reduction JSTypedLowering::ReduceJSCreateWithContext(Node* node) {
   DCHECK_EQ(IrOpcode::kJSCreateWithContext, node-opcode());
   Node* const input = NodeProperties::GetValueInput(node, 0);
+  Node* const closure = NodeProperties::GetValueInput(node, 1);
   Type* input_type = NodeProperties::GetBounds(input).upper;
-  if (FLAG_turbo_allocate  input_type-Is(Type::Receiver())) {
+  // The closure can be NumberConstant(0) if the closure is global code
+  // (rather than a function). We exclude that case here.
+  // TODO(jarin) Find a better way to check that the closure is a function.
+  if (FLAG_turbo_allocate  input_type-Is(Type::Receiver()) 
+  closure-opcode() != IrOpcode::kNumberConstant) {
 // JSCreateWithContext(o:receiver, f)
 Node* const effect = NodeProperties::GetEffectInput(node);
 Node* const control = NodeProperties::GetControlInput(node);
-Node* const closure = NodeProperties::GetValueInput(node, 1);
 Node* const context = NodeProperties::GetContextInput(node);
 Node* const load = graph()-NewNode(
 simplified()-LoadField(


--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Signal a blocked futex if the isolate is interrupted; don't busy-wait (issue 1230303005 by bi...@chromium.org)

[jarin]

On 2015/08/19 17:36:23, binji wrote:

https://codereview.chromium.org/1230303005/diff/120001/src/execution.cc
File src/execution.cc (right):



https://codereview.chromium.org/1230303005/diff/120001/src/execution.cc#newcode418

src/execution.cc:418: if (isolate_-futex_wait_list_node()-waiting()) {
On 2015/08/19 at 05:26:05, Jarin wrote:
 Could we perhaps pull the waiting() check into NotifyWake?

 Then the waiting accessors could be private (and perhaps we could even  
pull

the waiting_ flag into the critical section and make it non-atomic).


My thought here was to do a quick check to avoid taking a lock if there  
is no

futex waiting (which is the common case). What do you think?


Yeah, that's what I thought. However, locks are cheap, interrupts are rare  
and

atomics are scary. Up to you.

https://codereview.chromium.org/1230303005/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [turbofan] Do not optimize with-context allocation in global code. (issue 1292383007 by ja...@chromium.org)

[jarin]

https://codereview.chromium.org/1292383007/diff/1/src/compiler/js-typed-lowering.cc
File src/compiler/js-typed-lowering.cc (right):

https://codereview.chromium.org/1292383007/diff/1/src/compiler/js-typed-lowering.cc#newcode1238
src/compiler/js-typed-lowering.cc:1238: Node* const closure =
NodeProperties::GetValueInput(node, 1);
On 2015/08/19 14:19:24, Michael Starzinger wrote:

The same can happen for block contexts as well.


Done.

https://codereview.chromium.org/1292383007/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [simd.js] Macro-ize more SIMD code. (issue 1302513004 by bbu...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1302513004/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Signal a blocked futex if the isolate is interrupted; don't busy-wait (issue 1230303005 by bi...@chromium.org)

[jarin]

lgtm with a question.


https://codereview.chromium.org/1230303005/diff/120001/src/execution.cc
File src/execution.cc (right):

https://codereview.chromium.org/1230303005/diff/120001/src/execution.cc#newcode418
src/execution.cc:418: if (isolate_-futex_wait_list_node()-waiting()) {
Could we perhaps pull the waiting() check into NotifyWake?

Then the waiting accessors could be private (and perhaps we could even
pull the waiting_ flag into the critical section and make it
non-atomic).

https://codereview.chromium.org/1230303005/diff/120001/src/futex-emulation.h
File src/futex-emulation.h (right):

https://codereview.chromium.org/1230303005/diff/120001/src/futex-emulation.h#newcode44
src/futex-emulation.h:44: waiting_(false),
waiting_(0)

https://codereview.chromium.org/1230303005/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [turbofan]: Fix bug in register hinting (issue 1289313003 by da...@chromium.org)

[jarin]

lgtm. Thanks!

https://codereview.chromium.org/1289313003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [turbofan] Handle void return in simplified-lowering.cc. (issue 1296933002 by tit...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1296933002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [simd.js] Macro-ize more SIMD code. (issue 1293533003 by bbu...@chromium.org)

[jarin]

lgtm.

I cannot shake the feeling that the hydrogen part could look much better if  
we

had a better way to write else-if cascades.

https://codereview.chromium.org/1293533003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [turbofan] Support unboxed float and double stack parameters and add tests. (issue 1291113003 by tit...@chromium.org)

[jarin]

lgtm.

However, I am not a big fan of the 'if (Node* node = ...)' pattern,  
especially
when there is a 'node' variable in the outer scope, so it can be easily  
misread

as if (node == ...)'.

https://codereview.chromium.org/1291113003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [d8] Fix flakiness when calling quit() with isolates (issue 1294913005 by bi...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1294913005/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] [turbofan] Remove the output_index field that was unused in Node::Use. (issue 1294913003 by ja...@chromium.org)

[jarin]

Reviewers: Michael Starzinger,

Message:
Could you take a look, please?

Description:
[turbofan] Remove the output_index field that was unused in Node::Use.

BUG=

Please review this at https://codereview.chromium.org/1294913003/

Base URL: https://chromium.googlesource.com/v8/v8.git@master

Affected files (+2, -2 lines):
  M src/compiler/node.h


Index: src/compiler/node.h
diff --git a/src/compiler/node.h b/src/compiler/node.h
index  
a638df343df61c1063785ee446e769b3522f1fa7..e6c9f23fdc4daf1385bad1e11febe321e0fbacc8  
100644

--- a/src/compiler/node.h
+++ b/src/compiler/node.h
@@ -210,7 +210,6 @@ class Node final {
 uint32_t bit_field_;

 int input_index() const { return InputIndexField::decode(bit_field_); }
-int output_index() const { return  
OutputIndexField::decode(bit_field_); }

 bool is_inline_use() const { return InlineField::decode(bit_field_); }
 Node** input_ptr() {
   int index = input_index();
@@ -229,7 +228,8 @@ class Node final {

 typedef BitFieldbool, 0, 1 InlineField;
 typedef BitFieldunsigned, 1, 17 InputIndexField;
-typedef BitFieldunsigned, 17, 14 OutputIndexField;
+// Leaving some space in the bitset in case we ever decide to record
+// the output index.
   };


//



--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [turbofan]: Unify referencing of stack slots (issue 1261923007 by da...@chromium.org)

[jarin]

almost lgtm.


https://codereview.chromium.org/1261923007/diff/290027/src/compiler/frame.h
File src/compiler/frame.h (right):

https://codereview.chromium.org/1261923007/diff/290027/src/compiler/frame.h#newcode125
src/compiler/frame.h:125: ++spilled_callee_register_slot_count_ +=
count;
How about 'frame_slot_count_ += count + 1;'? I am not even sure what you
do here is well-defined.

https://codereview.chromium.org/1261923007/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Bug in bitfield in Node.h

[jarin]

Looking at the code closer, the OutputIndexField is never really used. 
Perhaps we can remove this...

Thanks for reporting!

- Jaro.

On Sunday, August 16, 2015 at 3:26:12 AM UTC+2, Aaron Gray wrote:

 Hi,

 I am pretty sure I have found a bug in Node.have on line 232 in 
 OutputIndexField it starts at offset 17 instead of an index of 18.

 Regards,
 Aaron



-- 
-- 
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups 
v8-dev group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [strong] Simplify (and sortof optimize) string addition for strong mode. (issue 1287203002 by bmeu...@chromium.org)

[jarin]

lgtm with a nit and a question.


https://codereview.chromium.org/1287203002/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/1287203002/diff/1/src/hydrogen.cc#newcode10907
src/hydrogen.cc:10907: if (left_type-Is(Type::String())) {
Could not you say here

if (left_type-Is(Type::String()) || is_strong(strength)) {
  left = BuildCheckString(left);
}

And then below say

if (!left_type-Is(Type::String())  !is_strong(strength)) {
  DCHECK(right_type-Is(Type::String()));
  if (left_type-Is(Type::Number())) {
...

https://codereview.chromium.org/1287203002/diff/1/src/hydrogen.cc#newcode10920
src/hydrogen.cc:10920: // In strong mode, if the left hand side of an
additition is a string,
additition - addition (Here and below.)

https://codereview.chromium.org/1287203002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [runtime] Remove useless IN builtin. (issue 1295433002 by bmeu...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1295433002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [turbofan] Propagate representation information from call descriptors in SimplifiedLowering. (issue 1292033002 by tit...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1292033002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Use TimeTicks instead of Time in FutexEmulation::Wait. (issue 1285723003 by bi...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1285723003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Revert of [turbofan] Various fixes to allow unboxed doubles as arguments in registers and on the stack. (issue 1284853002 by yang...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1284853002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Signal a blocked futex if the isolate is interrupted; don't busy-wait (issue 1230303005 by bi...@chromium.org)

[jarin]

One question below.

Still, could you explain why it deadlocked?


https://codereview.chromium.org/1230303005/diff/80001/src/futex-emulation.cc
File src/futex-emulation.cc (right):

https://codereview.chromium.org/1230303005/diff/80001/src/futex-emulation.cc#newcode138
src/futex-emulation.cc:138: if (node-CheckInterruptedAndClear()) {
Why can't you read and clear the interrupt flag while still holding the
lock? That way, you would not have to resort to the CASing, no? (You
could also use lexically scoped guards.)

https://codereview.chromium.org/1230303005/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] [Atomics] Fix compile failure in clang/win build in runtime-atomics.cc (issue 1287543004 by bi...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1287543004/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [turbofan] Drop V8_TURBOFAN_BACKEND and V8_TURBOFAN_TARGET defines. (issue 1282763002 by bmeu...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1282763002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [runtime] Simplify TO_INT32/TO_UINT32 abstract operations. (issue 1275013004 by bmeu...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1275013004/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [runtime] Remove premature optimization from ToPrimitive. (issue 1278873007 by bmeu...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1278873007/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [turbofan] Stand-alone deferred block splitting. (issue 1271703002 by mtro...@chromium.org)

[jarin]

I think I understand now how the spilling works, but it is very subtle.  
Once you

add explanatory comments, it will be good to go.

What are the performance numbers?


https://codereview.chromium.org/1271703002/diff/80001/src/compiler/register-allocator.cc
File src/compiler/register-allocator.cc (right):

https://codereview.chromium.org/1271703002/diff/80001/src/compiler/register-allocator.cc#newcode408
src/compiler/register-allocator.cc:408: // the range and control flow
connection mechanism instead.
Please elaborate on how this uses the control flow connection mechanism.

This incremental build-up of various moves is in my opinion the worst
part of the register allocator (in terms of understandability and
maintainability), and this CL makes it worse. It should be explained in
the comments.

https://codereview.chromium.org/1271703002/diff/80001/src/compiler/register-allocator.h
File src/compiler/register-allocator.h (right):

https://codereview.chromium.org/1271703002/diff/80001/src/compiler/register-allocator.h#newcode417
src/compiler/register-allocator.h:417: // in which it may be worse to
spill in it.
The comment seems to be out of date (it refers to the ranges with just
one spilled child).

https://codereview.chromium.org/1271703002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [turbofan] Stand-alone deferred block splitting. (issue 1271703002 by mtro...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1271703002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [turbofan] Debug-time helpers for regalloc data structures (issue 1280483002 by mtro...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1280483002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [turbofan] Various fixes to allow unboxed doubles as arguments in registers and on the stack. (issue 1263033004 by tit...@chromium.org)

[jarin]

lgtm




https://codereview.chromium.org/1263033004/diff/60001/src/compiler/raw-machine-assembler.h
File src/compiler/raw-machine-assembler.h (right):

https://codereview.chromium.org/1263033004/diff/60001/src/compiler/raw-machine-assembler.h#newcode482
src/compiler/raw-machine-assembler.h:482: // Call a given call
descriptor and the given arguments.
Please add an comment about the required length of the 'args' array
(from the code it looks like the length should be
desc-GetMachineSignature()-parameter_count()).

https://codereview.chromium.org/1263033004/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [turbofan] Stand-alone deferred block splitting. (issue 1271703002 by mtro...@chromium.org)

[jarin]

https://codereview.chromium.org/1271703002/diff/60001/src/compiler/register-allocator.cc
File src/compiler/register-allocator.cc (right):

https://codereview.chromium.org/1271703002/diff/60001/src/compiler/register-allocator.cc#newcode392
src/compiler/register-allocator.cc:392: move-AddMove(assigned,
spill_operand);
On 2015/08/04 20:34:46, Mircea Trofin wrote:

On 2015/08/04 19:39:43, Jarin wrote:
 Still confused about this - where do we actually spill the deferred

block with

a
 call? This move seems to be only inserted for ranges that are not

spilled.


Correct. It sets up the stage for the register allocator. The register

allocator

will make the decision of where to spill, but now it will treat the

segments

over deferred ranges separately from the rest of the range.


I am sorry, that explanation did not really help me understand. As far
as I understand, CommitSpillsAtDefinition runs after the actual
allocation, so I do not see how this can setup the stage for register
allocation. Could you explain? Where exactly is the place that adds the
spilling move for deferred blocks with calls?

Or are you saying that the pre-processing phase inserts live ranges for
the deferred blocks so that the register allocator can spill those
ranges (and since those ranges begin in deferred blocks, they would get
the right spill)? I cannot see how this would work either because the
pre-processing seems to only create child ranges, so they would not
really get their own definitions.

https://codereview.chromium.org/1271703002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [turbofan] Handle void returns in instruction selector. (issue 1269183002 by tit...@chromium.org)

[jarin]

lgtm




https://codereview.chromium.org/1269183002/diff/1/src/compiler/instruction-selector.cc
File src/compiler/instruction-selector.cc (right):

https://codereview.chromium.org/1269183002/diff/1/src/compiler/instruction-selector.cc#newcode1004
src/compiler/instruction-selector.cc:1004: Emit(kArchRet, g.NoOutput(),
g.Use(value));
Any reason for calling g.Use(value)? This might keep that value alive
for longer than it needs to be.

https://codereview.chromium.org/1269183002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [turbofan] Preprocessing live ranges before register allocation. (issue 1256313003 by mtro...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1256313003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [d8 Workers] Throw when calling Worker constructor without new (issue 1260813008 by bi...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1260813008/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [turbofan] Stand-alone deferred block splitting. (issue 1271703002 by mtro...@chromium.org)

[jarin]

Unfortunately, I am quite confused about this change, so I have bunch of
questions.


https://codereview.chromium.org/1271703002/diff/60001/src/compiler/move-optimizer.cc
File src/compiler/move-optimizer.cc (right):

https://codereview.chromium.org/1271703002/diff/60001/src/compiler/move-optimizer.cc#newcode69
src/compiler/move-optimizer.cc:69: if (has_only_deferred) continue;
Could you explain why we skip the optimization here?

https://codereview.chromium.org/1271703002/diff/60001/src/compiler/pipeline.cc
File src/compiler/pipeline.cc (right):

https://codereview.chromium.org/1271703002/diff/60001/src/compiler/pipeline.cc#newcode1340
src/compiler/pipeline.cc:1340: // TODO(mtrofin): re-enable greedy once
we have bots for range preprocessing.
Do we really want to have a bot for range preprocessing? That seems to
be an overkill.

https://codereview.chromium.org/1271703002/diff/60001/src/compiler/preprocess-live-ranges.cc
File src/compiler/preprocess-live-ranges.cc (right):

https://codereview.chromium.org/1271703002/diff/60001/src/compiler/preprocess-live-ranges.cc#newcode83
src/compiler/preprocess-live-ranges.cc:83: int stop) {
stop - end

https://codereview.chromium.org/1271703002/diff/60001/src/compiler/preprocess-live-ranges.cc#newcode91
src/compiler/preprocess-live-ranges.cc:91: void
SplitRangeByClobberingDeferredBlocks(LiveRange* range,
The name is a bit confusing (it sounds like it clobbers deferred
blocks). Perhaps SplitRangeAtDeferredBlocksWithCalls?

https://codereview.chromium.org/1271703002/diff/60001/src/compiler/preprocess-live-ranges.cc#newcode112
src/compiler/preprocess-live-ranges.cc:112: if (last_index =
code-InstructionCount()) {
It could be more readable if you said if (last_index 
code-LastInstructionIndex()) last_index =
code-LastInstructionIndex();. With that, you could remove the
InstructionCount method.

https://codereview.chromium.org/1271703002/diff/60001/src/compiler/register-allocator.cc
File src/compiler/register-allocator.cc (right):

https://codereview.chromium.org/1271703002/diff/60001/src/compiler/register-allocator.cc#newcode392
src/compiler/register-allocator.cc:392: move-AddMove(assigned,
spill_operand);
Still confused about this - where do we actually spill the deferred
block with a call? This move seems to be only inserted for ranges that
are not spilled.

https://codereview.chromium.org/1271703002/diff/60001/src/compiler/register-allocator.h
File src/compiler/register-allocator.h (right):

https://codereview.chromium.org/1271703002/diff/60001/src/compiler/register-allocator.h#newcode456
src/compiler/register-allocator.h:456: bool
IsSpilledInSingleDeferredBlock() const {
I am really confused about this. The name refers to *single* deferred
block, but the TryCommitSpillInDeferredBlock method seems to set the
spilled_in_deferred_block_ flag even if there are multiple such blocks.

https://codereview.chromium.org/1271703002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [turbofan] Preprocessing live ranges before register allocation. (issue 1256313003 by mtro...@chromium.org)

[jarin]

On 2015/07/31 16:09:00, Mircea Trofin wrote:

Quick update: Benedikt is on vacation; I have just returned from one.  
Today, I

have been mostly catching up on email and fixing/triaging urgent issues. I
should be able to review your CLs tomorrow.

https://codereview.chromium.org/1256313003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] [deoptimizer] Do not pass arguments markers to the debugger. (issue 1263333002 by ja...@chromium.org)

[jarin]

Reviewers: Yang,

Message:
Could take a look, please?

It turns out the arguments object screw-up was caused by my deoptimizer
refactoring.

Description:
[deoptimizer] Do not pass arguments markers to the debugger.

This fixes a bug introduced by r28826 (Unify decoding of deoptimization
translations, https://codereview.chromium.org/1136223004), where we
started leaking arguments marker sentinel to the debugger, which would
then cause crashes. This change replaces the sentinel with the undefined
value in the debugger-inspectable frame.

BUG=chromium:514362
LOG=n
R=yang...@chromium.org

Please review this at https://codereview.chromium.org/126002/

Base URL: https://chromium.googlesource.com/v8/v8.git@master

Affected files (+53, -2 lines):
  M src/deoptimizer.cc
  A test/mjsunit/debug-materialized.js


Index: src/deoptimizer.cc
diff --git a/src/deoptimizer.cc b/src/deoptimizer.cc
index  
599962a204fe5a362334dd8ec1a014f76c7d04f6..d29cb6056347d63f30ef9643a833dd4f90c6cb59  
100644

--- a/src/deoptimizer.cc
+++ b/src/deoptimizer.cc
@@ -2266,7 +2266,12 @@  
DeoptimizedFrameInfo::DeoptimizedFrameInfo(Deoptimizer* deoptimizer,

   source_position_ = code-SourcePosition(pc);

   for (int i = 0; i  expression_count_; i++) {
-SetExpression(i, output_frame-GetExpression(i));
+Object* value = output_frame-GetExpression(i);
+// Replace materialization markers with the undefined value.
+if (value == deoptimizer-isolate()-heap()-arguments_marker()) {
+  value = deoptimizer-isolate()-heap()-undefined_value();
+}
+SetExpression(i, value);
   }

   if (has_arguments_adaptor) {
@@ -2277,7 +2282,12 @@  
DeoptimizedFrameInfo::DeoptimizedFrameInfo(Deoptimizer* deoptimizer,

   parameters_count_ = output_frame-ComputeParametersCount();
   parameters_ = new Object* [parameters_count_];
   for (int i = 0; i  parameters_count_; i++) {
-SetParameter(i, output_frame-GetParameter(i));
+Object* value = output_frame-GetParameter(i);
+// Replace materialization markers with the undefined value.
+if (value == deoptimizer-isolate()-heap()-arguments_marker()) {
+  value = deoptimizer-isolate()-heap()-undefined_value();
+}
+SetParameter(i, value);
   }
 }

Index: test/mjsunit/debug-materialized.js
diff --git a/test/mjsunit/debug-materialized.js  
b/test/mjsunit/debug-materialized.js

new file mode 100644
index  
..0b01b78df491fadee3187d74d8b9b0922aefd4a9

--- /dev/null
+++ b/test/mjsunit/debug-materialized.js
@@ -0,0 +1,41 @@
+// Copyright 2015 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --allow-natives-syntax --expose-debug-as debug
+
+function dbg(x) {
+  debugger;
+}
+
+function foo() {
+  arguments[0];
+  dbg();
+}
+
+function bar() {
+  var t = { a : 1 };
+  dbg();
+  return t.a;
+}
+
+foo(1);
+foo(1);
+bar(1);
+bar(1);
+%OptimizeFunctionOnNextCall(foo);
+%OptimizeFunctionOnNextCall(bar);
+
+var Debug = debug.Debug;
+Debug.setListener(function(event, exec_state, event_data, data) {
+  if (event != Debug.DebugEvent.Break) return;
+  for (var i = 0; i  exec_state.frameCount(); i++) {
+var f = exec_state.frame(i);
+for (var j = 0; j  f.localCount(); j++) {
+  print(' + f.localName(j) + ' =  + f.localValue(j).value());
+}
+  }
+});
+
+foo(1);
+bar(1);


--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [turbofan] Various fixes to allow unboxed doubles as arguments in registers and on the stack. (issue 1263033004 by tit...@chromium.org)

[jarin]

Looking good so far.


https://codereview.chromium.org/1263033004/diff/1/src/compiler/x64/code-generator-x64.cc
File src/compiler/x64/code-generator-x64.cc (right):

https://codereview.chromium.org/1263033004/diff/1/src/compiler/x64/code-generator-x64.cc#newcode1224
src/compiler/x64/code-generator-x64.cc:1224: __ subq(rsp, Immediate(8));
Maybe replace 8 with kDoubleSize?

https://codereview.chromium.org/1263033004/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] [deoptimizer] Fix the frame size calculation for debugger-inspectable frame construction. (issue 1264483008 by ja...@chromium.org)

[jarin]

Reviewers: Yang,

Message:
Could you take a look, please?

Description:
[deoptimizer] Fix the frame size calculation for debugger-inspectable frame
construction.

The calculation now takes into account the size of the arguments object
if it is present in the optimized frame.

(Yang, many thanks for the awesome repro!)

BUG=chromium:514362
LOG=N
R=yang...@chromium.org

Please review this at https://codereview.chromium.org/1264483008/

Base URL: https://chromium.googlesource.com/v8/v8.git@master

Affected files (+17, -11 lines):
  M src/deoptimizer.cc
  A + test/mjsunit/regress/regress-514362.js


Index: src/deoptimizer.cc
diff --git a/src/deoptimizer.cc b/src/deoptimizer.cc
index  
d29cb6056347d63f30ef9643a833dd4f90c6cb59..498a467fb3d064f6f7524851826757cc80d0cf76  
100644

--- a/src/deoptimizer.cc
+++ b/src/deoptimizer.cc
@@ -154,8 +154,13 @@ DeoptimizedFrameInfo*  
Deoptimizer::DebuggerInspectableFrame(

   // Always use the actual stack slots when calculating the fp to sp
   // delta adding two for the function and context.
   unsigned stack_slots = code-stack_slots();
+  DeoptimizationInputData* data =
+  DeoptimizationInputData::cast(code-deoptimization_data());
+  unsigned arguments_stack_height =
+  data-ArgumentsStackHeight(deoptimization_index)-value() *  
kPointerSize;

   unsigned fp_to_sp_delta = (stack_slots * kPointerSize) +
-  StandardFrameConstants::kFixedFrameSizeFromFp;
+StandardFrameConstants::kFixedFrameSizeFromFp +
+arguments_stack_height;

   Deoptimizer* deoptimizer = new Deoptimizer(isolate,
  function,
Index: test/mjsunit/regress/regress-514362.js
diff --git a/test/mjsunit/regress/regress-crbug-487289.js  
b/test/mjsunit/regress/regress-514362.js

similarity index 51%
copy from test/mjsunit/regress/regress-crbug-487289.js
copy to test/mjsunit/regress/regress-514362.js
index  
dbfb4041ca93465fcf579e2af52f6fd9f41d5481..f69cfecebe3893bcab6fecb0227c6e48aaebcb71  
100644

--- a/test/mjsunit/regress/regress-crbug-487289.js
+++ b/test/mjsunit/regress/regress-514362.js
@@ -2,19 +2,20 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.

-// Flags: --expose-debug-as debug
+// Flags: --allow-natives-syntax --expose-debug-as debug

-var Debug = debug.Debug;
-var receiver = null;
+function bar(x) { debugger; }
+function foo() { bar(arguments[0]); }
+function wrap() { return foo(1); }
+
+wrap();
+wrap();
+%OptimizeFunctionOnNextCall(wrap);

+var Debug = debug.Debug;
 Debug.setListener(function(event, exec_state, event_data, data) {
   if (event != Debug.DebugEvent.Break) return;
-  receiver = exec_state.frame(0).evaluate('this').value();
+  for (var i = 0; i  exec_state.frameCount(); i++) exec_state.frame(i);
 });

-function f() { debugger; }
-
-var expected = {};
-f.call(expected);
-
-assertEquals(expected, receiver);
+wrap();


--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [deoptimizer] Fix the frame size calculation for debugger-inspectable frame construction. (issue 1264483008 by ja...@chromium.org)

[jarin]

https://codereview.chromium.org/1264483008/diff/1/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://codereview.chromium.org/1264483008/diff/1/src/deoptimizer.cc#newcode160
src/deoptimizer.cc:160:
data-ArgumentsStackHeight(deoptimization_index)-value() *
kPointerSize;
On 2015/08/03 11:35:14, Yang wrote:

ComputeOutgoingArgumentSize() contains the same code. Can we somehow

avoid

having this duplicate code?


Done.

https://codereview.chromium.org/1264483008/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [d8 worker] Fix regression when serializing very large arraybuffer (issue 1264723002 by bi...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1264723002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Remove ExternalArray, derived types, and element kinds (issue 1254623002 by joc...@chromium.org)

[jarin]

lgtm




https://codereview.chromium.org/1254623002/diff/60001/test/cctest/compiler/test-run-properties.cc
File test/cctest/compiler/test-run-properties.cc (right):

https://codereview.chromium.org/1254623002/diff/60001/test/cctest/compiler/test-run-properties.cc#newcode25
test/cctest/compiler/test-run-properties.cc:25: // elements kind to get
coverage for both access patterns:
The colon does not really make sense and it is not clear what 'both'
means here.

https://codereview.chromium.org/1254623002/diff/60001/test/cctest/compiler/test-run-properties.cc#newcode88
test/cctest/compiler/test-run-properties.cc:88: // -
IsExternalArrayElementsKind(y)
Same here.

https://codereview.chromium.org/1254623002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [d8] Fix tsan bugs (issue 1252623003 by bi...@chromium.org)

[jarin]

lgtm. Love it!

https://codereview.chromium.org/1252623003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [d8 Workers] Fix bug creating Worker during main thread termination (issue 1255563002 by bi...@chromium.org)

[jarin]

lgtm.


https://codereview.chromium.org/1255563002/diff/40001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/1255563002/diff/40001/src/d8.cc#newcode780
src/d8.cc:780: }
How about factoring out the extraction of the worker from the args into
a helper function?

Also, could you DCHECK that InternalFielCount is 1? Just to exclude the
possibility that we deal with some stray object.

https://codereview.chromium.org/1255563002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Remove code for no longer present external array on any object API (issue 1249723005 by joc...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1249723005/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Signal a blocked futex if the isolate is interrupted; don't busy-wait (issue 1230303005 by bi...@chromium.org)

[jarin]

I thought it looked good to me, but the bots disagree. Could you take a  
look?

(It would be great if you could describe what went wrong.)

https://codereview.chromium.org/1230303005/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [d8] Fix tsan bugs (issue 1252623003 by bi...@chromium.org)

[jarin]

lgtm with bunch of questions.


https://codereview.chromium.org/1252623003/diff/20001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/1252623003/diff/20001/src/d8.cc#newcode338
src/d8.cc:338: options.set_script_executed(true);
It looks like the set_script_executed could be calculated from the
command line arguments: script_executed is true if there is an argument
not prefixed with '-' or if there is an '-e' argument. Perhaps we could
just do a walk over the arguments (basically cut-down version of
SourceGroup::Execute) rather than the Atomic hackery?

https://codereview.chromium.org/1252623003/diff/20001/src/d8.cc#newcode2554
src/d8.cc:2554: options.set_last_run(i == stress_runs - 1);
I am a bit confused about how this can be run in parallel with anything.
I thought that everything should be stopped at this point, no?

https://codereview.chromium.org/1252623003/diff/20001/src/d8.cc#newcode2564
src/d8.cc:2564: options.set_last_run(i == stress_runs - 1);
Same here.

https://codereview.chromium.org/1252623003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [turbofan] Remove bloated GraphBuilder base class. (issue 1252093002 by mstarzin...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1252093002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [turbofan]: Elide extra move when accessting stack or frame register (issue 1242303005 by da...@chromium.org)

[jarin]

lgtm.

I am wondering whether the extra complexity is going to pay for itself. The
special casing OperandGenerator::Use looks funky (but I do not know of a  
better

way to do this).

https://codereview.chromium.org/1242303005/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [d8 Workers] Fix bug creating Worker during main thread termination (issue 1255563002 by bi...@chromium.org)

[jarin]

One question. If this is answered, it looks good.


https://codereview.chromium.org/1255563002/diff/20001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/1255563002/diff/20001/src/d8.cc#newcode1842
src/d8.cc:1842: state_ = TERMINATED;
What if we call quit() from one worker and worker.terminate() from
another one? Cannot there be a data race?

https://codereview.chromium.org/1255563002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [turbofan] Add some documenting comments to RawMachineAssembler. (issue 1248263003 by mstarzin...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1248263003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Store offset between fixed typed array base and data start in object (issue 1248483007 by joc...@chromium.org)

[jarin]

Ouch, my eyes! lgtm.


https://codereview.chromium.org/1248483007/diff/20001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1248483007/diff/20001/src/heap/heap.cc#newcode4012
src/heap/heap.cc:4012: kHeapObjectTag),
Can't you say
ExternalReference::fixed_typed_array_base_data_offset().address() here?

https://codereview.chromium.org/1248483007/diff/20001/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/1248483007/diff/20001/src/ia32/lithium-codegen-ia32.cc#newcode4097
src/ia32/lithium-codegen-ia32.cc:4097: Comment(;;; LOL Store);
What is LOL?

https://codereview.chromium.org/1248483007/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [turbofan] Deferred block spilling heuristic - first step. (issue 1242123006 by mtro...@chromium.org)

[jarin]

https://codereview.chromium.org/1242123006/diff/40001/src/compiler/greedy-allocator.cc
File src/compiler/greedy-allocator.cc (right):

https://codereview.chromium.org/1242123006/diff/40001/src/compiler/greedy-allocator.cc#newcode87
src/compiler/greedy-allocator.cc:87: auto instr =
data-code()-InstructionAt(index);
Expand 'auto', please.

https://codereview.chromium.org/1242123006/diff/40001/src/compiler/greedy-allocator.cc#newcode88
src/compiler/greedy-allocator.cc:88: if (!instr-IsCall()) continue;
This does not really work because live ranges can (and  do) get
indirectly spilled by calls: If a call's position lies somewhere inside
one of the live ranges intervals, the call clobbers all registers and
thus makes it impossible for the live range to be allocated. That
happens even if the call is not in the live range's use list.

Just try this on the following example:

var f = (function() {
  use asm
  return function f(x, y) {
x = x|0;
y = y|0;
return x+y|0;
  }
})();

%OptimizeFunctionOnNextCall(f)
f(1, 2);

Here the live range corresponding to x should not be spilled on the fast
path, but it is. (The same goes for the context that lives in rsi.)

https://codereview.chromium.org/1242123006/diff/40001/src/compiler/greedy-allocator.cc#newcode102
src/compiler/greedy-allocator.cc:102: }
Why do we have the loop here? Cannot it happen that we fall off to a
completely unrelated basic block here?

https://codereview.chromium.org/1242123006/diff/40001/test/unittests/compiler/register-allocator-unittest.cc
File test/unittests/compiler/register-allocator-unittest.cc (right):

https://codereview.chromium.org/1242123006/diff/40001/test/unittests/compiler/register-allocator-unittest.cc#newcode534
test/unittests/compiler/register-allocator-unittest.cc:534: int expect_1
= FLAG_turbo_greedy_regalloc ? 3 : 1;
Could you please explain what the magic constants 1 and 3 are and link
them to the appropriate instructions?

https://codereview.chromium.org/1242123006/diff/40001/test/unittests/compiler/register-allocator-unittest.cc#newcode540
test/unittests/compiler/register-allocator-unittest.cc:540: EXPECT_EQ(0,
GetMoveCount(*moves));
Please explain what you are checking here, this will be next to
impossible to understand if it fails.

https://codereview.chromium.org/1242123006/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Use a lock in pages to synchronize sweeper threads to allow others to wait on concurrently swept pa… (issue 1244353002 by hpa...@chromium.org)

[jarin]

lgtm. Oh boy, this is ugly.

At this point, can't we just use a standard lock + condition variable  
mechanism
to protect and wait for the page's sweeping status? Would it be too slow?  
E.g.,
in Heap::CanMoveObjectStart we would have to lock inside the  
SweepingCompleted

check.

This funky combination of lock + atomic variable seems to be quite subtle.  
E.g.,

I do not like that WaitUntilSweepingCompleted only works if you know that
sweeping has started, otherwise you might be surprised. Perhaps you could  
add a

comment to WaitUntilSweepingCompleted that documents the proper usage?


https://codereview.chromium.org/1244353002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [turbofan] Unit tests for live range conflicts. (issue 1219063017 by mtro...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1219063017/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [turbofan] Unit tests for live range conflicts. (issue 1219063017 by mtro...@chromium.org)

[jarin]

https://codereview.chromium.org/1219063017/diff/180001/src/compiler/coalesced-live-ranges.cc
File src/compiler/coalesced-live-ranges.cc (right):

https://codereview.chromium.org/1219063017/diff/180001/src/compiler/coalesced-live-ranges.cc#newcode66
src/compiler/coalesced-live-ranges.cc:66: break;
This continue-break gymnastics looks awkward.
How about
if (pos_ != end) {
  DCHECK(QueryIntersectsAllocatedInterval());
  break;
}

Or possibly

if (pos_ != end) {
  DCHECK(QueryIntersectsAllocatedInterval());
  return;
}

(Then you can unconditionally Invalidate after the loop.)

https://codereview.chromium.org/1219063017/diff/180001/src/compiler/coalesced-live-ranges.cc#newcode96
src/compiler/coalesced-live-ranges.cc:96:
storage_-erase({interval-start(), interval-end(), nullptr});
Uniform initialization is banned in Chrome, apparently. Please equip
AllocationInterval with a constructor.

(See https://chromium-cpp.appspot.com. The reason for banning is
incorrect support in some versions of Visual Studio.)

https://codereview.chromium.org/1219063017/diff/180001/src/compiler/coalesced-live-ranges.cc#newcode119
src/compiler/coalesced-live-ranges.cc:119:
storage().insert({interval-start(), interval-end(), range});
Same here, no uniform initialization.

https://codereview.chromium.org/1219063017/diff/180001/src/compiler/coalesced-live-ranges.h
File src/compiler/coalesced-live-ranges.h (right):

https://codereview.chromium.org/1219063017/diff/180001/src/compiler/coalesced-live-ranges.h#newcode48
src/compiler/coalesced-live-ranges.h:48: LiveRange*
ClearCurrentAndGetNext() { return InternalGetNext(true); }
ClearCurrent seems ambiguous to me, it sounds like it is somehow
invalidating the current range.

How about EvictCurrentAndGetNext, then? (Or RemoveCurrentAndGetNext.)

https://codereview.chromium.org/1219063017/diff/180001/src/compiler/coalesced-live-ranges.h#newcode100
src/compiler/coalesced-live-ranges.h:100: IntervalStore* storage_;
Could we please move away from the 'storage_' name? It does not say at
all what the field is (every field is in essence storage). How about
intervals_?

https://codereview.chromium.org/1219063017/diff/180001/src/compiler/coalesced-live-ranges.h#newcode131
src/compiler/coalesced-live-ranges.h:131: bool
TestOnlyVerifyAllocationsAreValid() const;
TestOnlyVerifyAllocationsAreValid - VerifyAllocationsAreValidForTesting

The Chrome convention is to add the ForTesting suffix (and there are
some rough checks that aim to detect calls from production code to
testing code (i.e., methods without the ForTesting suffix calling
methods with the ForTesting suffix).

https://codereview.chromium.org/1219063017/diff/180001/src/compiler/coalesced-live-ranges.h#newcode136
src/compiler/coalesced-live-ranges.h:136: IntervalStore storage() {
return storage_; }
Again, could we rename storage - intervals.

https://codereview.chromium.org/1219063017/diff/180001/test/unittests/compiler/coalesced-live-ranges-unittest.cc
File test/unittests/compiler/coalesced-live-ranges-unittest.cc (right):

https://codereview.chromium.org/1219063017/diff/180001/test/unittests/compiler/coalesced-live-ranges-unittest.cc#newcode36
test/unittests/compiler/coalesced-live-ranges-unittest.cc:36: auto pair
= pairs_[i];
auto - Interval

https://codereview.chromium.org/1219063017/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Add support for adding an external and a tagged pointer (issue 1244693002 by joc...@chromium.org)

[jarin]

Scary stuff. It looks ok, but fairly brittle. Adding mstarzinger@ to have a
look.


https://codereview.chromium.org/1244693002/diff/20001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/1244693002/diff/20001/src/hydrogen-instructions.h#newcode4890
src/hydrogen-instructions.h:4890: right-representation().IsTagged()) {
This looks quite brittle, perhaps we should introduce some other flag
that differentiates the special HAdd from ordinary ones.

You could then use the flag to do the right thing in
HAdd::RequiredInputRepresentation.

https://codereview.chromium.org/1244693002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Add support for adding an external and a tagged pointer (issue 1244693002 by joc...@chromium.org)

[jarin]

lgtm.

Even though it would be even better if

- all the DCHECKs took into account the new flag.
- the flag was explicitly passed on the HAdd constructor (ideally as an  
enum,

with the default value being the normal add) and then DCHECK that the
representations are in sync with the constructor flag. I just want to avoid  
the

situation that somebody uses the dangerous HAdd by accident.

https://codereview.chromium.org/1244693002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Add support for adding an external and a tagged pointer (issue 1244693002 by joc...@chromium.org)

[jarin]

Looks even better, I still have two suggestions, but it is up to you.


https://codereview.chromium.org/1244693002/diff/80001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/1244693002/diff/80001/src/hydrogen-instructions.h#newcode4918
src/hydrogen-instructions.h:4918: }
Hmm, I was imagining you would drive it the other way:

if (external_add_type == AddOfExternalAndTagged) {
  DCHECK(left-representation().IsExternal());
  DCHECK(right-representation().IsTagged());
  SetDependsOnFlag(kNewSpacePromotion);
} else {
  ...
}

https://codereview.chromium.org/1244693002/diff/80001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

https://codereview.chromium.org/1244693002/diff/80001/src/ia32/lithium-ia32.cc#newcode1636
src/ia32/lithium-ia32.cc:1636:
instr-right()-representation().IsTagged()));
Maybe you want to factor out the two DCHECK into a separate method, up
to you.

Something like

bool HAdd::IsConsistentExternalRepresentation() {
  return  left()-representation().IsExternal() 
 ((external_add_type() == AddOfExternalAndInt32 
  right()-representation().IsInteger32()) ||
 (external_add_type() == AddOfExternalAndTagged 
  right()-representation().IsTagged()));
}

https://codereview.chromium.org/1244693002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Add support for adding an external and a tagged pointer (issue 1244693002 by joc...@chromium.org)

[jarin]

looks good with one final nit


https://codereview.chromium.org/1244693002/diff/120001/src/mips/lithium-mips.cc
File src/mips/lithium-mips.cc (right):

https://codereview.chromium.org/1244693002/diff/120001/src/mips/lithium-mips.cc#newcode1625
src/mips/lithium-mips.cc:1625:
DCHECK(instr-left()-representation().IsExternal());
You can leave out DCHECK(instr-left()-representation().IsExternal()),
it is covered by IsConsistentExternalRepresentation.

https://codereview.chromium.org/1244693002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Atomics Futex API (issue 1208933006 by bi...@chromium.org)

[jarin]

lgtm. (but we should revisit the busy wait at some point.)


https://codereview.chromium.org/1208933006/diff/240001/src/futex-emulation.cc
File src/futex-emulation.cc (right):

https://codereview.chromium.org/1208933006/diff/240001/src/futex-emulation.cc#newcode63
src/futex-emulation.cc:63: // TODO(binji): How long should this be?
Should it be configurable?
No idea. Ideally, we should have some notification from the interrupt
mechanism so that you do not have to busy-wait here. Perhaps you could
add a TODO here that reminds us to fix the busy wait?

https://codereview.chromium.org/1208933006/diff/240001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/1208933006/diff/240001/test/cctest/test-api.cc#newcode21866
test/cctest/test-api.cc:21866: };
Thanks for the test, you are my hero!

https://codereview.chromium.org/1208933006/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: d8: Leak context_mutex_ so it will never be destroyed while locked (issue 1240553003 by bi...@chromium.org)

[jarin]

lgtm.

https://codereview.chromium.org/1240553003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Atomics Futex API (issue 1208933006 by bi...@chromium.org)

[jarin]

On 2015/07/15 18:50:53, jochen wrote:
yeah, it's essentially the same as while (1) {} - nothing gets rendered,  
and

after a while you'll get the tab unresponsive, kill? dialog.


With while(1) {} you still get stack checks, so you can interrupt (e.g.,  
from

the debugger or some such). Here, there is no possibility to interrupt.

https://codereview.chromium.org/1208933006/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Atomics Futex API (issue 1208933006 by bi...@chromium.org)

[jarin]

Adding jochen to discuss the blocking of main thread.


https://codereview.chromium.org/1208933006/diff/180001/src/futex-emulation.cc
File src/futex-emulation.cc (right):

https://codereview.chromium.org/1208933006/diff/180001/src/futex-emulation.cc#newcode80
src/futex-emulation.cc:80: node-cond_.Wait(mutex_.Pointer());
Benedikt pointed out that we cannot block the main thread indefinitely.

Either we block only for small time quanta and allow interruption from
the embedder (i.e., Blink) or we find somehow let the embedder do the
waiting.

https://codereview.chromium.org/1208933006/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Fix runtime-atomics for Win 10 SDK and remove volatile (issue 1228063005 by brucedaw...@chromium.org)

[jarin]

lgtm.

While I agree that volatile has no useful meaning for multi-threading, it  
does

prevents some wilder compiler optimizations, and in the absence of any other
synchronization primitives it might be better than nothing. I think it is  
fine
to leave out 'volatile' here because the Interlocked* intrinsics should  
prevent

bad optimizations.

https://codereview.chromium.org/1228063005/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [osr] Increase Code::profiler_ticks to 28 bits. (issue 1224173003 by bmeu...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1224173003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: d8 workers: make sure Shell::Quit is only called once (issue 1230403003 by bi...@chromium.org)

[jarin]

lgtm.

Even though it might be better to put the 'once' variable on the Shell class
(the Chrome build likes to complain about static initialization).

https://codereview.chromium.org/1230403003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: d8: Fix some TSAN bugs (issue 1226143003 by bi...@chromium.org)

[jarin]

lgtm




https://codereview.chromium.org/1226143003/diff/60001/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/1226143003/diff/60001/src/d8.cc#newcode1693
src/d8.cc:1693: if (base::NoBarrier_CompareAndSwap(join_called_, false,
true) == false) {
It looks like join_called_ is only used for debugging. Could we perhaps
just DCHECK here? (Hmm, the compiler might complain about join_called_
not being used.)

https://codereview.chromium.org/1226143003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] In Atomics API, convert operands to numbers before calling runtime. (issue 1232243002 by bi...@chromium.org)

[jarin]

lgtm




https://codereview.chromium.org/1232243002/diff/1/test/mjsunit/harmony/atomics.js
File test/mjsunit/harmony/atomics.js (right):

https://codereview.chromium.org/1232243002/diff/1/test/mjsunit/harmony/atomics.js#newcode166
test/mjsunit/harmony/atomics.js:166: [false, true,
undefined].forEach(function(v) {
Maybe you should also try an object with a toString method (which should
be called by the toNumber conversion).

https://codereview.chromium.org/1232243002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: [arm] CheckConstPool between TurboFan instructions. (issue 1232343002 by bmeu...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1232343002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Remove more uses of the deprecated EnumSet template class. (issue 1229233002 by bmeu...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1229233002/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: d8: Fix some TSAN bugs (issue 1226143003 by bi...@chromium.org)

[jarin]

https://codereview.chromium.org/1226143003/diff/1/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/1226143003/diff/1/src/d8.cc#newcode1703
src/d8.cc:1703: case STOPPED:
Why do we want to retry when STOPPED?

Actually, I am even confused why we are trying to go through the
TERMINATING state in WaitForThread. Why (instead of calling Terminate)
don't you CAS RUNNING-JOINING?

It would be helpful if you could describe what was wrong with the
original state machine.

https://codereview.chromium.org/1226143003/diff/1/src/d8.cc#newcode1708
src/d8.cc:1708: case JOINING:
Is not this only called when the shell finishes, so there should not be
anyone else joining?

https://codereview.chromium.org/1226143003/diff/1/src/d8.cc#newcode2187
src/d8.cc:2187: char* p = new char[length];
How about using std::vectorchar here, instead of the naked pointer?

https://codereview.chromium.org/1226143003/diff/1/src/d8.cc#newcode2268
src/d8.cc:2268: // Now that all workers are terminated, we can re-enable
Worker creation.
I know this has not be introduced by this CL, but why do we want to
re-enable worker creation? At this point, the process is about to die,
no?

https://codereview.chromium.org/1226143003/diff/1/src/d8.h
File src/d8.h (right):

https://codereview.chromium.org/1226143003/diff/1/src/d8.h#newcode261
src/d8.h:261: enum State { IDLE, RUNNING, TERMINATING, STOPPED, JOINING,
JOINED };
Could we have some descriptions of what the states mean?

https://codereview.chromium.org/1226143003/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Atomics Futex API (issue 1208933006 by bi...@chromium.org)

[jarin]

First batch of comments.


https://codereview.chromium.org/1208933006/diff/160001/src/futex-emulation.cc
File src/futex-emulation.cc (right):

https://codereview.chromium.org/1208933006/diff/160001/src/futex-emulation.cc#newcode18
src/futex-emulation.cc:18: FutexWaitList FutexEmulation::wait_list_;
Static initializers are not allowed by Chrome. You should use the
LazyInstance template.

https://codereview.chromium.org/1208933006/diff/160001/src/futex-emulation.cc#newcode39
src/futex-emulation.cc:39: if (node-prev_)
braces, please.

https://codereview.chromium.org/1208933006/diff/160001/src/futex-emulation.h
File src/futex-emulation.h (right):

https://codereview.chromium.org/1208933006/diff/160001/src/futex-emulation.h#newcode38
src/futex-emulation.h:38: : prev_(NULL),
NULL - nullptr in new code, please.

https://codereview.chromium.org/1208933006/diff/160001/src/harmony-atomics.js
File src/harmony-atomics.js (right):

https://codereview.chromium.org/1208933006/diff/160001/src/harmony-atomics.js#newcode136
src/harmony-atomics.js:136: if (index  0 || index = ia.length) {
I believe you should not rely on the length property here because it
could be monkey-patched. You should use %_TypedArrayGetLength(ia) here.

Could you add tests with monkey-patched length and byteLength properties
for all the atomic ops?

https://codereview.chromium.org/1208933006/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[v8-dev] Re: Fix cluster-fuzz found regression with d8 Workers (issue 1220193004 by bi...@chromium.org)

[jarin]

lgtm



https://codereview.chromium.org/1220193004/

--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
--- 
You received this message because you are subscribed to the Google Groups v8-dev group.

To unsubscribe from this group and stop receiving emails from it, send an email 
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.