Bug 159545 - Refactor JSStack to only be the stack data structure for the C Loop.
Summary: Refactor JSStack to only be the stack data structure for the C Loop.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on: 159544 159549
Blocks:
  Show dependency treegraph
 
Reported: 2016-07-07 22:08 PDT by Mark Lam
Modified: 2016-07-11 13:49 PDT (History)
7 users (show)

See Also:


Attachments
proposed patch. (93.18 KB, patch)
2016-07-11 11:45 PDT, Mark Lam
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2016-07-07 22:08:40 PDT
Currently, JSStack acts both as an abstraction for JS stack for both JIT and C Loop engines, as well as the stack data structure for the C Loop.  Per my discussion with Geoff, we'll refactor it to only be the C Loop stack data structure.  All the other parts of the JSStack can be moved elsewhere, or removed if no longer necessary.  With that, we'll also rename JSStack to CLoopStack.
Comment 1 Mark Lam 2016-07-11 11:45:26 PDT
Created attachment 283328 [details]
proposed patch.

Let's try this on the EWS.
Comment 2 WebKit Commit Bot 2016-07-11 11:47:40 PDT
Attachment 283328 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/interpreter/CLoopStack.h:40:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Mark Lam 2016-07-11 12:24:27 PDT
Comment on attachment 283328 [details]
proposed patch.

EWS bots are green, and local C Loop test (with test bot test configuration) shows no regression.  Let's get this reviewed.
Comment 4 Geoffrey Garen 2016-07-11 12:29:59 PDT
Comment on attachment 283328 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=283328&action=review

r=me

> Source/JavaScriptCore/interpreter/CLoopStackInlines.h:39
> +inline bool CLoopStack::ensureCapacityFor(Register* newTopOfStack)
>  {
> -#if !ENABLE(JIT)
>      return grow(newTopOfStack);

No need for one function whose only job is to call another function.
Comment 5 Mark Lam 2016-07-11 13:46:50 PDT
(In reply to comment #4)
> r=me

Thanks.


> > Source/JavaScriptCore/interpreter/CLoopStackInlines.h:39
> > +inline bool CLoopStack::ensureCapacityFor(Register* newTopOfStack)
> >  {
> > -#if !ENABLE(JIT)
> >      return grow(newTopOfStack);
> 
> No need for one function whose only job is to call another function.

I consolidated grow() into ensureCapactityFor(), deleted the old grow(), and renamed growSlowCase() to grow().
Comment 6 Mark Lam 2016-07-11 13:49:39 PDT
Landed in r203081: <http://trac.webkit.org/r203081>.