724736 - Move js::StringBuffer into js/src/vm/StringBuffer{-inl.h,.cpp}

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

It would be nice for reference purposes if StringBuffer were in its own file.  (And in any case, it certainly shouldn't be in jsstrinlines.h, seeing as we're trying to move things out of the one universal namespace of js/src/ into more-distinct places.)

This is fairly easy code motion, and it'll give a little insight into how our string code fits together as well.

Comment 1

[Luke Wagner [:luke]]

Yes, please; it's annoying to track down things that live entirely in *inlines.h files.

Comment 2

[Charles Chan]

Just to be clear, is the intention to move StringBuffer class out of jsstrinlines.h and into jsstringbuffer.h and jsstringbuffer.cpp?

(This is my first time working on the code, this looks like an easy one to get started.)

Comment 3

[Jeff Walden [:Waldo]]

As you can see js/src is kind of crowded, so we've been making a push to move things into organized subdirectories (builtin, frontend, gc, vm, and so on) and gradually empty out js/src/ itself.  Here, we want to add js/src/vm/StringBuffer{.h,-inl.h,.cpp} files.  That clear things up?

Comment 4

[Charles Chan]

Attachment: Refactor StringBuffer

Thanks for the guidance. Here's my first attempt to move StringBuffer into js/src/vm/StringBuffer{.h,.cpp}.

A few more questions:
1) Is the StringBuffer-inl.h file supposed to include the inline functions only? 
2) Who should is the appropriate module owner for code review?

Thanks.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 600737 [details] [diff] [review]
Refactor StringBuffer

Review of attachment 600737 [details] [diff] [review]:
-----------------------------------------------------------------

https://wiki.mozilla.org/JavaScript:Hackers is a good list of JS hackers.  More generally, if you take a look at who's reviewed changes to the relevant files in the past, those are good people to ask to review.  This time at least, I'm happy to take this -- I'll get back with comments in the next couple days or so, if not sooner.

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 600737 [details] [diff] [review]
Refactor StringBuffer

Review of attachment 600737 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Charles Chan from comment #4)
> 1) Is the StringBuffer-inl.h file supposed to include the inline functions
> only? 

*-inl.h are mostly a workaround for cyclic dependencies.  If you have an inline method in class C that depends on class B, but you also have an inline method in class B that depends on class C, you can declare the classes and methods in C.h and B.h.  Then you can implement the actual inline methods in C-inl.h (which #includes C.h and B.h) and B-inl.h (which #includes B.h and C.h).

If there are no such cyclic dependencies, not using an -inl.h file is best as it's simpler than defining stuff in two phases.  StringBuffer itself doesn't have any such dependencies as best as I recall, but I think String itself does, and StringBuffer will depend on String-inl.h.

So in conclusion:

* inline methods should be defined inside the class definition in the *.h header, if possible
* but if an inline method depends on something only defined in a *-inl.h header, that inline method should be defined in *-inl.h

Yeah, it's complicated.  :-\  An arguable defect of C++ compared to newer languages (and older ones), sadly.

::: js/src/jsstrinlines.h
@@ +47,5 @@
>  
>  #include "jscntxtinlines.h"
>  #include "jsgcinlines.h"
>  #include "vm/String-inl.h"
> +#include "vm/StringBuffer.h"

Unless something in jsstrinlines.h depends on StringBuffer -- which it looks like it doesn't, after the moves here -- you should get rid of this #include, and you should make only the files that need a StringBuffer #include it.  Part of the reason to move StringBuffer into its own files is that it means changes to StringBuffer will only cause things that use StringBuffer to need recompilation.  If you include this file here still, that reason won't really pay off.

@@ +52,2 @@
>  
> +namespace js{

Keep the old "namespace js {" spacing.

::: js/src/vm/StringBuffer.cpp
@@ +34,5 @@
> + * and other provisions required by the GPL or the LGPL. If you do not delete
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */

These days we use the MPL2 header, rather than the MPL1.1/GPL2/LGPL2.1 tri-license.  You'll want to copy that header from here:

http://www.mozilla.org/MPL/headers/

@@ +47,5 @@
> +
> +#include "jscntxtinlines.h"
> +#include "jsgcinlines.h"
> +#include "vm/String-inl.h"
> +*/

You'll want to remove commented-out bits like this from the final patch.

::: js/src/vm/StringBuffer.h
@@ +34,5 @@
> + * and other provisions required by the GPL or the LGPL. If you do not delete
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */

New license header here too.

@@ +68,5 @@
> + */
> +class StringBuffer
> +{
> +    /* cb's buffer is taken by the new string so use ContextAllocPolicy. */
> +    typedef Vector<jschar, 32, ContextAllocPolicy> CharBuffer;

Since this is directly using js::Vector, you should make sure to directly include "js/Vector.h" above.

It should be possible to #include "vm/StringBuffer.h" without needing to include anything else -- just as should be the case for any other header.  (Not that we always hold to that standard, but we should be...)

@@ +83,5 @@
> +
> +  public:
> +    StringBuffer(JSContext *cx);
> +    bool reserve(size_t len);
> +    bool resize(size_t len);

One of the nice things about StringBuffer currently is that most methods on it (save for the "big hammers" that only happen rarely, like extracting a string from them) are inline (more precisely, their definitions are inline, and calling sites will be able to see those definitions) and potentially/likely faster as a result.  Your changes move all the methods out of line.  That probably will make things slower.  Most of these method declarations should be marked inline in the class definition.  The method definitions should be in vm/StringBuffer-inl.h (if they can't simply be directly in the class definition) and should be similarly marked.  Could you do that here, please?

@@ +143,5 @@
> +    if (v.isString())
> +        return sb.append(v.toString());
> +
> +    return ValueToStringBufferSlow(cx, v, sb);
> +}

This method will need to go in the -inl.h header, because it'll depend on being able to see the definition of js::StringBuffer::append(JSString*).

What platform are you compiling on, incidentally?  One thing to watch out for in any patch is that you might add a build warning.  Given that the current StringBuffer definition marks a bunch of methods as not inline, it's possible adding inline to all the right places may trigger some warnings, without broader changes.

Comment 7

[Charles Chan]

Attachment: Refactor StringBuffer-2

Jeff, thanks for your excellent explanation and guidance.

1) What does 'vm' stands for? (Javascript) 'virtual machine'?

2) I tried to addressed most of your review feedback. Please have a look at the new patch file.

3)
> What platform are you compiling on, incidentally?  One thing to watch out for 
> in any patch is that you might add a build warning.  Given that the current
> StringBuffer definition marks a bunch of methods as not inline, it's possible 
> adding inline to all the right places may trigger some warnings, without 
> broader changes.

I am running on Ubuntu. To fix these build warnings, do I need to add '-Werror' somewhere? .mozconfig? Also, is there an automated/system for me to test the  compilation on other platforms?

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 600784 [details] [diff] [review]
Refactor StringBuffer-2

Review of attachment 600784 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/StringBuffer-inl.h
@@ +6,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + *
> + * ***** END LICENSE BLOCK ***** */
> +
> +#include "StringBuffer.h"

"vm/StringBuffer.h"

@@ +13,5 @@
> +
> +inline
> +StringBuffer::StringBuffer(JSContext *cx)
> +  : cb(cx)
> +{}

I think this can go into the normal header, as well as

@@ +16,5 @@
> +  : cb(cx)
> +{}
> +
> +inline bool
> +StringBuffer::reserve(size_t len)

this

@@ +24,5 @@
> +    return cb.reserve(len);
> +}
> +
> +inline bool
> +StringBuffer::resize(size_t len)

this

@@ +32,5 @@
> +    return cb.resize(len);
> +}
> +
> +inline bool
> +StringBuffer::append(const jschar c)

this

@@ +40,5 @@
> +    return cb.append(c);
> +}
> +
> +inline bool
> +StringBuffer::append(const jschar *chars, size_t len)

this

@@ +48,5 @@
> +    return cb.append(chars, len);
> +}
> +
> +inline bool
> +StringBuffer::append(const jschar *begin, const jschar *end)

this

@@ +72,5 @@
> +    return cb.append(str->chars(), str->length());
> +}
> +
> +inline bool
> +StringBuffer::appendN(const jschar c, size_t n)

and this.

::: js/src/vm/StringBuffer.h
@@ +4,5 @@
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + *
> + * ***** END LICENSE BLOCK ***** */

There's no need for the ***** BEGIN/END LICENSE BLOCK ***** lines; just have a one-line comment for the emacs modeline, and the 3-line comment from <http://www.mozilla.org/MPL/headers/>. Same for -inl.h.

@@ +106,5 @@
> +    }
> +};
> +
> +extern bool
> +ValueToStringBufferSlow(JSContext *cx, const Value &v, StringBuffer &sb);

I think this can go into the -inl.h, where it's needed.

Comment 9

[Charles Chan]

Attachment: Refactor StringBuffer-3

Fixed the issues identified in the review.

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 600828 [details] [diff] [review]
Refactor StringBuffer-3

Review of attachment 600828 [details] [diff] [review]:
-----------------------------------------------------------------

Looking better, looking better.  Still some more changes needed -- StringBuffer.cpp and the -inl inclusion bits most importantly.

"vm" stands for "virtual machine", yes, in the sense of the abstract JS execution machine we're implementing.  It's a little screwy if you ask me, but I guess it's the common way to name the thing.

There's an automated build/testing system known as "try server" -- once we've got something that passes review I'll throw it at that and we'll see how things go.  Once you've got your foot in the door a bit we can look into getting you direct access to it.

As far as compiler warnings go, it's part what you can see locally, part what tinderbox displays, and part intuition about what constructs are likely to trigger warnings -- using inline methods without providing definitions for them, comparing signed and unsigned values, and other things.  Unfortunately some people are curiously opposed to making compilers more vocal about pointing out warnings, despite nobody liking them, so for now writing warning-free code involves a bit of muddling through things usually.

::: js/src/jsonparser.cpp
@@ +42,5 @@
>  #include "jsnum.h"
>  #include "jsonparser.h"
>  
>  #include "jsobjinlines.h"
>  #include "jsstrinlines.h"

Can this be removed?  Do so if you can.

::: js/src/vm/StringBuffer.h
@@ +11,5 @@
> +
> +#include "js/Vector.h"
> +
> +//#include "jsatom.h"
> +//#include "jsstr.h"

Remove these.

@@ +14,5 @@
> +//#include "jsatom.h"
> +//#include "jsstr.h"
> +
> +#include "jscntxtinlines.h"
> +//#include "jsgcinlines.h"

And this.

@@ +15,5 @@
> +//#include "jsstr.h"
> +
> +#include "jscntxtinlines.h"
> +//#include "jsgcinlines.h"
> +#include "vm/String-inl.h"

Hmm.  I believe our rule has been that -inl headers may include other -inl headers, but non-inl headers may not include -inl headers.  So this should move to StringBuffer-inl.h, and any methods defined below which rely on this should move to StringBuffer-inl.h as well.

@@ +44,5 @@
> +
> +    static inline bool checkLength(JSContext *cx, size_t length);
> +    bool checkLength(size_t length);
> +    JSContext *context() const { return cb.allocPolicy().context(); }
> +    jschar *extractWellSized();

This method is defined in jsstr.cpp.  The new convention is that .h/-inl.h/.cpp totally define something -- so this method should move to a new StringBuffer.cpp file.  Same for the other two StringBuffer methods in jsstr.cpp.

@@ +59,5 @@
> +    bool append(const jschar *begin, const jschar *end);
> +    bool append(JSString *str);
> +    bool append(JSLinearString *str);
> +    bool appendN(const jschar c, size_t n);
> +    bool appendInflated(const char *cstr, size_t len);

Since all of these methods are defined as inlines, add the inline specifier to all of them (and to all other methods in this class, save for the three defined in jsstr.cpp now, StringBuffer.cpp post-patch -- oh, and the methods that are also defined inline, like infallibleAppend, don't need changing either).

@@ +75,5 @@
> +    void infallibleAppendN(const jschar c, size_t n) {
> +        cb.infallibleAppendN(c, n);
> +    }
> +
> +    JSAtom *atomize(uintN flags = 0);

A patch just landed today which removed the uintN type -- it's now just |unsigned|.  Probably a few other places that need this tweak now, too...

Comment 11

[Jeff Walden [:Waldo]]

Oh, you should see build warnings (should they happen) in the normal course of building SpiderMonkey.  The easiest way to make them more apparent is to build with "make -s", which causes make to not print compiler command lines and such things (except in case of failure).  Then the only output will be file names being compiled, and compiler warnings from those files, if any.

Comment 12

[Charles Chan]

Thanks, I will try and re-submit the patch later this week. (Hopefully 'hg pull' won't create a lot of conflicts/headaches)

Comment 13

[Charles Chan]

Attachment: Refactor StringBuffer-4

One more try. Please review.

Thanks.

Comment 14

[Charles Chan]

Attachment: Refactor StringBuffer-5

Sorry, forgot the inclusion guard for StringBuffer-inl.h, re-submit patch for review.

Comment 15

[Jeff Walden [:Waldo]]

Comment on attachment 602668 [details] [diff] [review]
Refactor StringBuffer-5

Review of attachment 602668 [details] [diff] [review]:
-----------------------------------------------------------------

I made the fixes noted below and pushed the patch to inbound -- thanks for the patch!

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e5e3d16a492

In the future, you should make sure the patches you post include proper attribution.  (I added the relevant details this time around.)  Adding this to your ~/.hgrc should do the trick for your name in future patches (existing patches you can then fix up using |qrefresh -U|):

[ui]
username = Your Real Name <[e-mail]>

[defaults]
qnew = -U

Also it's best if your patches include a reasonable summary.  If you're using Mercurial queues, you can add a summary with |qrefresh -e|.

::: js/src/jsdate.cpp
@@ -76,5 @@
>  #include "vm/GlobalObject.h"
>  
>  #include "jsinferinlines.h"
>  #include "jsobjinlines.h"
> -#include "jsstrinlines.h"

Excellent!  This is *exactly* why smaller headers with reduced dependency sets are worth it.

::: js/src/jsexn.cpp
@@ -67,5 @@
>  #include "vm/GlobalObject.h"
>  
>  #include "jsinferinlines.h"
>  #include "jsobjinlines.h"
> -#include "jsstrinlines.h"

\o/

::: js/src/jsnum.cpp
@@ -77,5 @@
>  #include "jsatominlines.h"
>  #include "jsinferinlines.h"
>  #include "jsnuminlines.h"
>  #include "jsobjinlines.h"
> -#include "jsstrinlines.h"

\o/ (and I'm going to stop noting these, but pretend a \o/ accompanies every one)

::: js/src/jsstrinlines.h
@@ +175,5 @@
> +    PodCopy(storage, chars, length);
> +    storage[length] = 0;
> +    Probes::createString(cx, str, length);
> +    return str;
> +}

This method is better added to vm/String-inl.h.

::: js/src/vm/StringBuffer-inl.h
@@ +13,5 @@
> +
> +inline bool
> +StringBuffer::checkLength(size_t length)
> +{
> +    return JSString::validateLength(context(), length);

This call is going to need to see the definition of this method in vm/String-inl.h, so you'll need to include that.

::: js/src/vm/StringBuffer.cpp
@@ +5,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "vm/StringBuffer.h"
> +
> +#include "jsstrinlines.h"

Longer-term we'll want to get rid of this dependency (and NewShortString(?) should move to String-inl.h), but for now this is fine.

::: js/src/vm/StringBuffer.h
@@ +121,5 @@
> +
> +inline bool
> +StringBuffer::appendInflated(const char *cstr, size_t cstrlen)
> +{
> +    size_t lengthBefore = length();

This looks like it needs the definition of StringBuffer::length, so that should move above this in this header.

Comment 16

[Ed Morley [:emorley]]

These two have just popped up on dev.tree-management:

Regression :( Dromaeo (jslib) decrease 1.69% on XP Mozilla-Inbound-Non-PGO
--------------------------------------------------------------------------
    Previous: avg 146.910 stddev 1.088 of 30 runs up to revision 980483ace374
    New     : avg 144.425 stddev 0.374 of 5 runs since revision 8b8c280f4df0
    Change  : -2.484 (1.69% / z=2.284)
    Graph   : http://mzl.la/w8e7c7

Changeset range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=980483ace374&tochange=8b8c280f4df0


Talos Regression :( Dromaeo (jslib) decrease 2.1% on Win7 Mozilla-Inbound-Non-PGO
Regression :( Dromaeo (jslib) decrease 2.1% on Win7 Mozilla-Inbound-Non-PGO
---------------------------------------------------------------------------
    Previous: avg 146.047 stddev 1.105 of 30 runs up to revision 980483ace374
    New     : avg 142.986 stddev 0.320 of 5 runs since revision 8b8c280f4df0
    Change  : -3.061 (2.1% / z=2.770)
    Graph   : http://mzl.la/xECsxX

Changeset range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=980483ace374&tochange=8b8c280f4df0

Comment 17

[Ed Morley [:emorley]]

Also now:

Regression :( Dromaeo (jslib) decrease 2.58% on XP Mozilla-Inbound
------------------------------------------------------------------
    Previous: avg 160.645 stddev 0.973 of 30 runs up to revision a61b13046123
    New     : avg 156.500 stddev 0.733 of 5 runs since revision 1448a8de40a8
    Change  : -4.146 (2.58% / z=4.259)
    Graph   : http://mzl.la/zxZvLi

Changeset range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a61b13046123&tochange=1448a8de40a8


(Albeit larger regression-range since PGO test runs less frequent)

Comment 18

[Charles Chan]

Do these test results mean the performance is faster now? ('New' values are less than 'Previous')

Comment 19

[Ed Morley [:emorley]]

Sadly in this instance, higher is better - so these are regressions.

Waldo, can you advise Charles as to what might be causing these?

Comment 20

[Matt Brubeck (:mbrubeck)]

This patch has landed in mozilla-central. It will be in tomorrow's nightly builds. Thanks!
https://hg.mozilla.org/mozilla-central/rev/4e5e3d16a492

Comment 21

[Jeff Walden [:Waldo]]

Hmm.  This was just code motion, in theory, so it shouldn't have affected anything perfy.  Modulo compiler sadnesses, of course.  I wonder if maybe something became not-inline that used to be inline, possibly; will look into that first.

Comment 22

[Charles Chan]

I checked out all the 'inline' functions which used to be in jsstrinlines.h. They all looked identical after the move to StringBuffer{.h/-inl.h/.cpp).

<< begin list >> 
static inline bool checkLength(JSContext *cx, size_t length);
inline bool checkLength(size_t length);
explicit inline StringBuffer(JSContext *cx);
inline int length() const;
inline StringBuffer::StringBuffer(JSContext *cx)
inline bool StringBuffer::reserve(size_t len)
inline bool StringBuffer::resize(size_t len)
inline bool StringBuffer::append(const jschar c)
inline bool StringBuffer::append(const jschar *chars, size_t len)
inline bool StringBuffer::append(const jschar *begin, const jschar *end)
inline bool StringBuffer::append(JSString *str)
inline bool StringBuffer::append(JSLinearString *str)
inline bool StringBuffer::appendN(const jschar c, size_t n)
inline bool StringBuffer::appendInflated(const char *cstr, size_t cstrlen)
inline int StringBuffer::length() const
inline bool StringBuffer::checkLength(size_t length)
inline bool ValueToStringBuffer(JSContext *cx, const Value &v, StringBuffer &sb)
<< end list >> 

Where can I find out more about these regression tests? Would like to see what function it is calling to follow the breadcrumbs. Any other suggestions?

Comment 23

[Jeff Walden [:Waldo]]

All the tests mentioned here are Dromaeo:

http://dromaeo.com/

I think there's a downloadable copy that's the best way to run it.

That said, notice where the regression happened: on Windows.  Different versions of Windows, but (I believe) the same compiler.  You mentioned being on Linux, so I doubt there's much that you could do to investigate this.  :-\  It'd be just so much guesswork, without being able to test/run/likely disassemble parts of a Windows binary.

All this said, there's another possibility: that the regression is not fairly attributed to this specific patch.  Given that this was just code motion, this is the conclusion I'm most inclined to.  Also, Dromaeo is not the most representative benchmark in the world; thus its importance is somewhat less than its placement in our tests might indicate.  So I'm going to say that we should concentrate on other bugs rather than worry about this.

Speaking of other bugs, now that this one's done, have you thought about what might interest you next?  #jsapi on Mozilla IRC would be more than happy to discuss bugs you might consider, if you want to swing by sometime.  We're most active from about 08:00 to 17:00 on weekdays in the time zone Bugzilla displays comments in, by default, but people are around outside those hours too, or could juggle things somewhat to be on outside those hours if necessary.

Comment 24

[Charles Chan]

Thanks Jeff, I am still exploring different areas of FF. I will try to join the IRC some time. (One thing I am interested in building Firefox for Windows 8 Metro and I have emailed Asa Dotzler, who has hook me up with the relevant developers ...)

Next up on my list: Bug 730872.

Cheers.