641048 - proper constructor for JSThread/JSThreadData

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

Currently JSThread and JSThreadData rely on js_calloc and explicit finish calls. That forces other classes that are used as fields in JSThreadData to follow the pattern instead of using normal constructor/destructor pairs. It would be nice to fix this and use standard C++ machinery.

This is a follow up for the bug 633219 comment 3.

Comment 1

[Igor Bukanov]

Attachment: v1

The patch provides proper constructor/destructors to JSThread and related classes. It also changes GSNCache to use the HashMap instead of the older JSDHashTable. To make sure that GSNCache::purge releases the maximum amount of memory the patch adds HashMap::finish method that releases the internal table and requires to call the init method again.

Comment 2

[Igor Bukanov]

Attachment: v2

v1 cannot compile on Windows due to instantiation of the inlined HashMap destructor when xpc includes the JS internal headers. v2 fixes this via adding a couple of friend API to expose the necessary functionality without including the whole JS stuff.

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 519131 [details] [diff] [review]
v2

Great stuff, this will continue to save frustration for years to come.  I also like the PointerHasher change.

Few high level comments/questions:

Could you move the init() and destructor of JSThreadData/JSThread/StackSpace into jscntxt.cpp?  Its nice for the definitions to be adjacent and there doesn't seem to be any perf need for inline-ness.

Next, instead of adding a finish() member to the hash table, perhaps just have a LazilyConstructed<HashMap<...> > and call destroy() when you want to purge?  This way it is clear that the hash table and all its memory go away (and thus need to be re-constructed) vs. the finish/init rules.  This tactic has already been used a couple of times for Vector in lieu of a Vector::finish().

Why were inline HashMap destructors causing problems?  Regardless, I like removing js internal #includes from xpconnect; perhaps though mrbkap or gal should review the names and the way those APIs are cut.

Comment 4

[Igor Bukanov]

(In reply to comment #3)
> Next, instead of adding a finish() member to the hash table, perhaps just have
> a LazilyConstructed<HashMap<...> > and call destroy() when you want to purge?

The current pattern for a class with the init method is to ensure that the destructor can be called when the init was not called. This implies that instances of the class have the well-defined constructed-but-not-yet-initialized state.

IMO providing an explicit finish method for such classes to transition to this state is more attractive than LazilyConstructed for three reasons. First it avoids the need to call both the constructor and the init method during the lazy initialization minimizing the amount of code to write and to read. Second it avoids the runtime penalty associated with the maintaining the extra state that LazilyConstructed has to do. Third it improves diagnostic behavior. For example, the proposed finish method for the hashtable increments, not clears, the mutation and generation counters. 

So I view LazilyConstructed as a way to provide the init, isInitialized, finish methods for the classes that currently lack these methods. 

> This tactic has already
> been used a couple of times for Vector in lieu of a Vector::finish().

It just means (again, IMO) that our Vector should gain the finish method.

> Why were inline HashMap destructors causing problems?

The compiler errors were during instantiating of SystemAllocPolicy and not available moz_free implementation. I guess some include headers were not provided.

Comment 5

[Luke Wagner [:luke]]

(In reply to comment #4)
Ok, agreed.

Comment 6

[Igor Bukanov]

Attachment: v3

The new version moves the init and destructor methods into jscntxt.cpp.

Comment 7

[Igor Bukanov]

Comment on attachment 519778 [details] [diff] [review]
v3

Asking for extra review of changes to jsfriendapi.* to avoid including js internals in js/src/xpconnect/loader

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 519778 [details] [diff] [review]
v3

Cool, r+ (modulo the mrbkap parts) with final nits:

For the same reason as ThreadData, could you pull the StackSpace constructor out the header and put it above StackSpace::init?  Also, to reduce churn and keep the StackSpace:: members adjacent, put StackSpace::init back to where it was and put ~StackSpace next to it?

>diff --git a/js/src/jshashtable.h b/js/src/jshashtable.h
>+    void finish()
>+    {

void finish() {

Comment 9

[Igor Bukanov]

Attachment: v4

Asking for en extra review for jsfriendsapi.* and mozJSSubScriptLoader.cpp changes.

Comment 10

[Igor Bukanov]

Attachment: v5

hg pull brought yet another compilation problem in another file. The reason is that depending on the configuration options we include headers that does 

#define free() moz_free() 

Now, depending on the header inclusion order moz_free may not resolve. To avoid that problem once and forever I renamed malloc/realloc/free in the allocation policy classes into mallocMem/reallocMem/freeMem.

Comment 11

[Luke Wagner [:luke]]

Comment on attachment 521188 [details] [diff] [review]
v5

In bug 643548 Paul is about to rename all the moz_*-conflicting names (by appending a _) so that we don't need the crazy macro magic in JS and we don't have the weird issues you are having.  Could you wait (probably not more than a day) and just land v4?

Comment 12

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/34d87d26a315 - landing v4 adjusted for the new allocation/free name from the bug 643548.

Comment 13

[Nicholas Nethercote [inactive]]

And this is why we need warnings-as-errors... there are bazillions of these warnings now:

../../src/js/src/jshashtable.h:770: warning: right shift count >= width of type

Comment 14

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/84e734e6e8ab - followup to fix the compilation warning

Comment 15

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/34d87d26a315
http://hg.mozilla.org/mozilla-central/rev/84e734e6e8ab