Bug 159451 – Rename VM stack limit fields to better describe their purpose.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

This is in preparation for an upcoming patch that changes what stack limit values are used under various circumstances.  This patch aims to do minimal work to rename the fields so that it will be easier to reason about the upcoming patch.

[Mark Lam]

Created attachment 282852 [details]
proposed patch.

[Keith Miller]

Comment on attachment 282852 [details]
proposed patch.

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

r=me with comment.

> Source/JavaScriptCore/ChangeLog:14
> +        In this patch, we will make the following changes:
> +
> +        1. Renamed VM::m_stackLimit to VM::m_jsCPUStackLimit.

Nit: You use the future tense for "we will make" then you use the past tense in the list.

[Mark Lam]

(In reply to comment #2)
> Nit: You use the future tense for "we will make" then you use the past tense
> in the list.

Thanks.  I fixed the tenses locally.  Will land soon.

[Mark Lam]

Landed in r202862: <http://trac.webkit.org/r202862>.

[Geoffrey Garen]

Comment on attachment 282852 [details]
proposed patch.

I find these names super confusing.

The CPU does not have a stack, so a "CPU stack limit" is a strange concept.

An "emulated stack" is an interesting concept, but nothing in the rest of the engine uses that phrase.

In general, solving a naming problem by adding more names is usually moving in the wrong direction.

My understanding is that we have three values:

(1) Stack limit. The true limit beyond which the OS will kill us.

(2) Stack limit with reserve. A more conservative limit we usually check in order to reserve some memory for host code.

(3) CLoop stack limit. A separate limit that only applies when we're running in LLInt and using the CLoop.

I think we should come up with names we like for (1), (2), and (3). We'll know that we have good names when the words we use to describe these things appear in their names.

[Geoffrey Garen]

(In reply to comment #5)
...And appear elsewhere in our code.

[Mark Lam]

(In reply to comment #5)
> Comment on attachment 282852 [details]
> proposed patch.
> 
> I find these names super confusing.
> 
> The CPU does not have a stack, so a "CPU stack limit" is a strange concept.

I'm not married to the term "CPUStack".  My purpose is to distinguish it from the "EmulatedStack" in that one is used by the CPU natively, and one is emulated / non-native by the interpreter.  I had toyed with "CStack" (as in used by C/C++ code) vs "CLoopStack" (as in used by the CLoop) but didn't like the sound of those.

How about "MachineStack" instead?  We have a file MachineStackMarker.h/cpp that applies the concept of "MachineStack" being the one used by C/C++ code.  I can contrast that with "EmulatedStack" or "CLoopStack".

 
> An "emulated stack" is an interesting concept, but nothing in the rest of
> the engine uses that phrase.

There is nothing in the rest of the engine that need to explicitly distinguish the CLoop / Emulated stack from the C++ / Machine stack.  Only this code does.  Hence, I don't think this should be a limiting factor.


> In general, solving a naming problem by adding more names is usually moving
> in the wrong direction.
> 
> My understanding is that we have three values:
> 
> (1) Stack limit. The true limit beyond which the OS will kill us.
> 
> (2) Stack limit with reserve. A more conservative limit we usually check in
> order to reserve some memory for host code.
> 
> (3) CLoop stack limit. A separate limit that only applies when we're running
> in LLInt and using the CLoop.

We have a term for (1) already.  It's the stack bounds.  Historically, we've been using "limit" to mean (2) or (3) depending on whether we are using the CLoop or not.  We could change these to use "ReserveLimit" or "AllowedLimit" or some such term, but I think that is not necessary because this is the only limit concept we use for stacks in the majority of our code, and it is nicer to write "Limit" then "ReserveLimit" (especially when we're not normally contrasting between the 2).

So, how about we leave (1) to StackBounds, and use "MachineStackLimit" for (2) and "EmulatedStackLimit" / "CLoopStackLimit" for (3)?

[Geoffrey Garen]

Let's consider CLoop first.

Our name for the CLoop stack is "JSStack". (See JSStack.h) That's why the limit was named "JSStack limit".

You've added a new name: EmulatedStack. So, now we have three names for the same thing: CLoop stack; JSStack; EmulatedStack.

As a matter of principle, I hope you'll agree that the point of a name is to identify a thing, and that having three names for the same thing contradicts that point and harms readability.

So, let's pick a name that we like and use it everywhere, so we can recover the value of naming things: identification.

I'm sympathetic to not liking "JSStack" and "JSStack limit" since they're pretty abstract, and they doesn't really communicate their unique use in the CLoop.

I suggest renaming "JSStack" to "CLoopStack" and renaming "JSStack limit" -- which is now called "emulated stack limit" to "CLoopStackLimit". 

You could also achieve one name by going the other way and renaming "JSStack" to "EmulatedStack" and "JSStack limit" to "EmulatedStackLimit". But that retains the problem that "emulated stack" is an abstract phrase that doesn't really say what it's used for.

[Mark Lam]

To follow up, Geoff and I talked thru the naming issue and the confusion with JSStack's role.  I'll address them in the following bugs:

https://bugs.webkit.org/show_bug.cgi?id=159544
Rename jsCPUStackLimit to osStackLimitWithReserve and jsEmulatedStackLimit to cloopStackLimit.

https://bugs.webkit.org/show_bug.cgi?id=159545
Refactor JSStack to only be the stack data structure for the C Loop.