Bug 22609 - Provide a build-time choice when generating hash tables for properties of built-in DOM objects
Summary: Provide a build-time choice when generating hash tables for properties of bui...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2008-12-02 16:26 PST by David Kilzer (:ddkilzer)
Modified: 2008-12-05 17:07 PST (History)
3 users (show)

See Also:


Attachments
Patch v1 (18.09 KB, patch)
2008-12-02 17:07 PST, David Kilzer (:ddkilzer)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2008-12-02 16:26:17 PST
*SUMMARY
WebKit r31147 <http://trac.webkit.org/changeset/31147> replaced a "compact" hash with a perfect hash for storing properties of built-in DOM objects.  Unfortunately, this tends to use more memory to store rather small amounts of data.  It would be nice to restore the original "compact" hash code and to provide a build-time choice to select the hashing algorithm.

* NOTES
<rdar://problem/6331749>
Comment 1 David Kilzer (:ddkilzer) 2008-12-02 17:07:59 PST
Created attachment 25694 [details]
Patch v1

Passes all JavaScriptCore tests and Layout Tests with ENABLE_PERFECT_HASH_SIZE set to 0 in JavaScriptCore/runtime/Lookup.h.
Comment 2 Darin Adler 2008-12-04 09:21:54 PST
Comment on attachment 25694 [details]
Patch v1

> +#if ENABLE(PERFECT_HASH_SIZE)
>  void HashTable::createTable(JSGlobalData* globalData) const

Since both of these implementations share the same function name I think it would be more elegant to put the #if around the body of the function rather than the entire function.

> +// Define ENABLE_PERFECT_HASH_SIZE to 0 to save memory
> +#define ENABLE_PERFECT_HASH_SIZE 1

This is not a great comment. Why would anyone let the value be 1 if setting it to 0 saves memory?

r=me
Comment 3 David Kilzer (:ddkilzer) 2008-12-05 11:55:31 PST
(In reply to comment #2)
> (From update of attachment 25694 [details] [review])
> > +#if ENABLE(PERFECT_HASH_SIZE)
> >  void HashTable::createTable(JSGlobalData* globalData) const
> 
> Since both of these implementations share the same function name I think it
> would be more elegant to put the #if around the body of the function rather
> than the entire function.

Will do.

> > +// Define ENABLE_PERFECT_HASH_SIZE to 0 to save memory
> > +#define ENABLE_PERFECT_HASH_SIZE 1
> 
> This is not a great comment. Why would anyone let the value be 1 if setting it
> to 0 saves memory?

I'll describe the speed/memory trade-off instead when landing.

Comment 4 David Kilzer (:ddkilzer) 2008-12-05 17:01:52 PST
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 25694 [details] [review] [review])
> > > +#if ENABLE(PERFECT_HASH_SIZE)
> > >  void HashTable::createTable(JSGlobalData* globalData) const
> > 
> > Since both of these implementations share the same function name I think it
> > would be more elegant to put the #if around the body of the function rather
> > than the entire function.
> 
> Will do.

Will also make this change to this method in Lookup.h:

+         ALWAYS_INLINE const HashEntry* entry(const Identifier& identifier) const

Comment 5 David Kilzer (:ddkilzer) 2008-12-05 17:05:17 PST
$ git svn dcommit
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	JavaScriptCore/ChangeLog
	M	JavaScriptCore/create_hash_table
	M	JavaScriptCore/runtime/Lookup.cpp
	M	JavaScriptCore/runtime/Lookup.h
	M	JavaScriptCore/runtime/Structure.cpp
	M	WebCore/ChangeLog
	M	WebCore/bindings/scripts/CodeGeneratorJS.pm
Committed r39056

http://trac.webkit.org/changeset/39056

Comment 6 David Kilzer (:ddkilzer) 2008-12-05 17:07:35 PST
Follow-up ChangeLog entry fix:

$ git svn dcommit
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	JavaScriptCore/ChangeLog
	M	WebCore/ChangeLog
Committed r39057

http://trac.webkit.org/changeset/39057