proper constructor for JSThread/JSThreadData

RESOLVED FIXED

Status

()

enhancement
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: igor, Assigned: igor)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 attachments, 3 obsolete attachments)

Assignee

Description

8 years ago
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.
Assignee

Comment 1

8 years ago
Posted patch v1 (obsolete) — Splinter Review
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.
Attachment #518962 - Flags: review?(luke)
Assignee

Comment 2

8 years ago
Posted patch v2 (obsolete) — Splinter Review
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.
Attachment #518962 - Attachment is obsolete: true
Attachment #518962 - Flags: review?(luke)
Attachment #519131 - Flags: review?(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.

Updated

8 years ago
Attachment #519131 - Flags: review?(luke)
Assignee

Comment 4

8 years ago
(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.
(In reply to comment #4)
Ok, agreed.
Assignee

Comment 6

8 years ago
Posted patch v3 (obsolete) — Splinter Review
The new version moves the init and destructor methods into jscntxt.cpp.
Attachment #519131 - Attachment is obsolete: true
Attachment #519778 - Flags: review?(luke)
Assignee

Comment 7

8 years ago
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
Attachment #519778 - Flags: review?(mrbkap)
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() {
Attachment #519778 - Flags: review?(luke) → review+
Assignee

Comment 9

8 years ago
Posted patch v4Splinter Review
Asking for en extra review for jsfriendsapi.* and mozJSSubScriptLoader.cpp changes.
Attachment #519778 - Attachment is obsolete: true
Attachment #519778 - Flags: review?(mrbkap)
Attachment #519881 - Flags: review?(mrbkap)
Attachment #519881 - Flags: review?(mrbkap) → review+
Assignee

Comment 10

8 years ago
Posted patch v5Splinter Review
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.
Attachment #521188 - Flags: review?(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?
Attachment #521188 - Flags: review?(luke)
Assignee

Updated

8 years ago
Depends on: 643548
Assignee

Comment 12

8 years ago
http://hg.mozilla.org/tracemonkey/rev/34d87d26a315 - landing v4 adjusted for the new allocation/free name from the bug 643548.
Whiteboard: fixed-in-tracemonkey
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
Assignee

Comment 14

8 years ago
http://hg.mozilla.org/tracemonkey/rev/84e734e6e8ab - followup to fix the compilation warning
You need to log in before you can comment on or make changes to this bug.