Bug 120333 – Implement ES6 Map object

• Component: New Bugs
• Status: RESOLVED FIXED

[Oliver Hunt]

Implement ES6 Map object

[Oliver Hunt]

Created attachment 209694 [details]
Patch

[Oliver Hunt]

Comment on attachment 209694 [details]
Patch

Not intended for review (O(N) behavior in a "map" is probably unexpected)

[Oliver Hunt]

Created attachment 209787 [details]
Patch

[Oliver Hunt]

Created attachment 209811 [details]
Patch

[WebKit Commit Bot]

Attachment 209811 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/heap/CopyToken.h', u'Source/JavaScriptCore/runtime/CommonIdentifiers.h', u'Source/JavaScriptCore/runtime/JSGlobalObject.cpp', u'Source/JavaScriptCore/runtime/JSGlobalObject.h', u'Source/JavaScriptCore/runtime/MapConstructor.cpp', u'Source/JavaScriptCore/runtime/MapConstructor.h', u'Source/JavaScriptCore/runtime/MapData.cpp', u'Source/JavaScriptCore/runtime/MapData.h', u'Source/JavaScriptCore/runtime/MapInstance.cpp', u'Source/JavaScriptCore/runtime/MapInstance.h', u'Source/JavaScriptCore/runtime/MapPrototype.cpp', u'Source/JavaScriptCore/runtime/MapPrototype.h', u'Source/JavaScriptCore/runtime/VM.cpp']" exit_code: 1
Source/JavaScriptCore/runtime/MapPrototype.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/runtime/MapInstance.cpp:28:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Source/JavaScriptCore/runtime/MapInstance.cpp:37:  Missing space after ,  [whitespace/comma] [3]
Source/JavaScriptCore/runtime/MapData.h:76:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Oliver Hunt]

Created attachment 209916 [details]
Patch

[WebKit Commit Bot]

Attachment 209916 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/heap/CopyToken.h', u'Source/JavaScriptCore/runtime/CommonIdentifiers.h', u'Source/JavaScriptCore/runtime/JSGlobalObject.cpp', u'Source/JavaScriptCore/runtime/JSGlobalObject.h', u'Source/JavaScriptCore/runtime/MapConstructor.cpp', u'Source/JavaScriptCore/runtime/MapConstructor.h', u'Source/JavaScriptCore/runtime/MapData.cpp', u'Source/JavaScriptCore/runtime/MapData.h', u'Source/JavaScriptCore/runtime/MapInstance.cpp', u'Source/JavaScriptCore/runtime/MapInstance.h', u'Source/JavaScriptCore/runtime/MapPrototype.cpp', u'Source/JavaScriptCore/runtime/MapPrototype.h', u'Source/JavaScriptCore/runtime/VM.cpp']" exit_code: 1
Source/JavaScriptCore/runtime/MapPrototype.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/runtime/MapData.cpp:28:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/JavaScriptCore/runtime/MapData.cpp:30:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/JavaScriptCore/runtime/MapInstance.cpp:28:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/JavaScriptCore/runtime/MapInstance.cpp:30:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/JavaScriptCore/runtime/MapInstance.cpp:39:  Missing space after ,  [whitespace/comma] [3]
Source/JavaScriptCore/runtime/MapConstructor.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/runtime/MapData.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/runtime/MapData.h:83:  Missing space inside { }.  [whitespace/braces] [5]
Source/JavaScriptCore/runtime/MapData.h:115:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/MapData.h:115:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/MapData.h:116:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/MapData.h:117:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/MapData.h:136:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 14 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 209916 [details]
Patch

Attachment 209916 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1625327

[Early Warning System Bot]

Comment on attachment 209916 [details]
Patch

Attachment 209916 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1605345

[Early Warning System Bot]

Comment on attachment 209916 [details]
Patch

Attachment 209916 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1625330

[Build Bot]

Comment on attachment 209916 [details]
Patch

Attachment 209916 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1625328

[Build Bot]

Comment on attachment 209916 [details]
Patch

Attachment 209916 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1624340

[EFL EWS Bot]

Comment on attachment 209916 [details]
Patch

Attachment 209916 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1626332

[EFL EWS Bot]

Comment on attachment 209916 [details]
Patch

Attachment 209916 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1627345

[kov's GTK+ EWS bot]

Comment on attachment 209916 [details]
Patch

Attachment 209916 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1591701

[Oliver Hunt]

Created attachment 209940 [details]
Patch

[WebKit Commit Bot]

Attachment 209940 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/fast/js/basic-map.html', u'LayoutTests/fast/js/script-tests/basic-map.js', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/heap/CopyToken.h', u'Source/JavaScriptCore/runtime/CommonIdentifiers.h', u'Source/JavaScriptCore/runtime/JSGlobalObject.cpp', u'Source/JavaScriptCore/runtime/JSGlobalObject.h', u'Source/JavaScriptCore/runtime/MapConstructor.cpp', u'Source/JavaScriptCore/runtime/MapConstructor.h', u'Source/JavaScriptCore/runtime/MapData.cpp', u'Source/JavaScriptCore/runtime/MapData.h', u'Source/JavaScriptCore/runtime/MapInstance.cpp', u'Source/JavaScriptCore/runtime/MapInstance.h', u'Source/JavaScriptCore/runtime/MapPrototype.cpp', u'Source/JavaScriptCore/runtime/MapPrototype.h', u'Source/JavaScriptCore/runtime/VM.cpp']" exit_code: 1
Source/JavaScriptCore/runtime/MapPrototype.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/runtime/MapPrototype.cpp:111:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/JavaScriptCore/runtime/MapData.cpp:28:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/JavaScriptCore/runtime/MapData.cpp:30:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/JavaScriptCore/runtime/MapInstance.cpp:28:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/JavaScriptCore/runtime/MapInstance.cpp:31:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/JavaScriptCore/runtime/MapInstance.cpp:40:  Missing space after ,  [whitespace/comma] [3]
Source/JavaScriptCore/runtime/MapConstructor.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/runtime/MapData.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/runtime/MapData.h:114:  Missing space inside { }.  [whitespace/braces] [5]
Source/JavaScriptCore/runtime/MapData.h:119:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/JavaScriptCore/runtime/MapData.h:158:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/MapData.h:158:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/MapData.h:159:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/MapData.h:160:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/MapData.h:205:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 16 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 209940 [details]
Patch

Attachment 209940 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1605440

[EFL EWS Bot]

Comment on attachment 209940 [details]
Patch

Attachment 209940 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1618459

[Early Warning System Bot]

Comment on attachment 209940 [details]
Patch

Attachment 209940 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1618460

[Build Bot]

Comment on attachment 209940 [details]
Patch

Attachment 209940 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1631028

[EFL EWS Bot]

Comment on attachment 209940 [details]
Patch

Attachment 209940 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1627406

[Build Bot]

Comment on attachment 209940 [details]
Patch

Attachment 209940 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1625426

[Build Bot]

Comment on attachment 209940 [details]
Patch

Attachment 209940 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1605439

[Oliver Hunt]

Created attachment 209944 [details]
Has full API spec and correctness

[WebKit Commit Bot]

Attachment 209944 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/fast/js/basic-map.html', u'LayoutTests/fast/js/script-tests/basic-map.js', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/heap/CopyToken.h', u'Source/JavaScriptCore/runtime/CommonIdentifiers.h', u'Source/JavaScriptCore/runtime/JSGlobalObject.cpp', u'Source/JavaScriptCore/runtime/JSGlobalObject.h', u'Source/JavaScriptCore/runtime/MapConstructor.cpp', u'Source/JavaScriptCore/runtime/MapConstructor.h', u'Source/JavaScriptCore/runtime/MapData.cpp', u'Source/JavaScriptCore/runtime/MapData.h', u'Source/JavaScriptCore/runtime/MapInstance.cpp', u'Source/JavaScriptCore/runtime/MapInstance.h', u'Source/JavaScriptCore/runtime/MapPrototype.cpp', u'Source/JavaScriptCore/runtime/MapPrototype.h', u'Source/JavaScriptCore/runtime/VM.cpp']" exit_code: 1
Source/JavaScriptCore/runtime/MapPrototype.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/runtime/MapData.cpp:28:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/JavaScriptCore/runtime/MapData.cpp:30:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/JavaScriptCore/runtime/MapInstance.cpp:28:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/JavaScriptCore/runtime/MapInstance.cpp:31:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/JavaScriptCore/runtime/MapInstance.cpp:40:  Missing space after ,  [whitespace/comma] [3]
Source/JavaScriptCore/runtime/MapConstructor.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/runtime/MapData.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/runtime/MapData.h:114:  Missing space inside { }.  [whitespace/braces] [5]
Source/JavaScriptCore/runtime/MapData.h:119:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/JavaScriptCore/runtime/MapData.h:158:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/MapData.h:158:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/MapData.h:159:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/MapData.h:160:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/MapData.h:205:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 15 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 209944 [details]
Has full API spec and correctness

Attachment 209944 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1626437

[EFL EWS Bot]

Comment on attachment 209944 [details]
Has full API spec and correctness

Attachment 209944 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1638040

[Early Warning System Bot]

Comment on attachment 209944 [details]
Has full API spec and correctness

Attachment 209944 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1625441

[kov's GTK+ EWS bot]

Comment on attachment 209944 [details]
Has full API spec and correctness

Attachment 209944 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1625444

[EFL EWS Bot]

Comment on attachment 209944 [details]
Has full API spec and correctness

Attachment 209944 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1624453

[Build Bot]

Comment on attachment 209944 [details]
Has full API spec and correctness

Attachment 209944 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1635038

[Build Bot]

Comment on attachment 209944 [details]
Has full API spec and correctness

Attachment 209944 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1639044

[Sam Weinig]

Comment on attachment 209944 [details]
Has full API spec and correctness

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

> Source/JavaScriptCore/runtime/MapConstructor.cpp:56
> +        return JSValue::encode(throwTypeError(exec, ASCIILiteral("Map constructor does not accept arguments")));
> +    if (!exec->argument(1).isUndefined())
> +        return JSValue::encode(throwError(exec, createRangeError(exec, WTF::ASCIILiteral("Invalid comparator function"))));
> +

On one line you use ASCIILiteral, the other WTF::ASCIILiteral! Make up your mind!

> Source/JavaScriptCore/runtime/MapConstructor.h:39
> +    static MapConstructor* create(ExecState* exec, JSGlobalObject* globalObject, Structure* structure, MapPrototype* functionPrototype)

This could probably take a VM& rather than an ExecState*.

> Source/JavaScriptCore/runtime/MapPrototype.cpp:65
> +    JSC_NATIVE_FUNCTION(vm.propertyNames->clear, mapProtoFuncClear, DontEnum, 0);
> +    JSC_NATIVE_FUNCTION(vm.propertyNames->deleteKeyword, mapProtoFuncDelete, DontEnum, 1);
> +    JSC_NATIVE_FUNCTION(vm.propertyNames->forEach, mapProtoFuncForEach, DontEnum, 1);
> +    JSC_NATIVE_FUNCTION(vm.propertyNames->get, mapProtoFuncGet, DontEnum, 1);
> +    JSC_NATIVE_FUNCTION(vm.propertyNames->has, mapProtoFuncHas, DontEnum, 1);
> +    JSC_NATIVE_FUNCTION(vm.propertyNames->set, mapProtoFuncSet, DontEnum, 2);

Why set them directly rather than use a lut?

[Geoffrey Garen]

Comment on attachment 209944 [details]
Has full API spec and correctness

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

Looks good. I have a few comments below, and it looks like this patch isn't quite building yet.

> Source/JavaScriptCore/ChangeLog:45
> +        * JavaScriptCore.xcodeproj/project.pbxproj:
> +        * runtime/CommonIdentifiers.h:
> +        * runtime/JSGlobalObject.cpp:
> +        (JSC::JSGlobalObject::reset):
> +        * runtime/JSGlobalObject.h:
> +        (JSC::JSGlobalObject::mapStructure):
> +        * runtime/MapConstructor.cpp: Added.
> +        (JSC::MapConstructor::finishCreation):
> +        (JSC::constructMap):
> +        (JSC::MapConstructor::getConstructData):
> +        (JSC::MapConstructor::getCallData):
> +        * runtime/MapConstructor.h: Added.
> +        (JSC::MapConstructor::create):
> +        (JSC::MapConstructor::createStructure):
> +        (JSC::MapConstructor::MapConstructor):
> +        * runtime/MapData.cpp: Added.
> +        (JSC::MapData::ensureSpaceForAppend):
> +        (JSC::MapData::find):
> +        (JSC::MapData::set):
> +        (JSC::MapData::get):
> +        (JSC::MapData::remove):
> +        (JSC::MapData::visitChildren):
> +        * runtime/MapData.h: Added.
> +        (JSC::MapData::create):
> +        (JSC::MapData::createStructure):
> +        (JSC::MapData::contains):
> +        (JSC::MapData::size):
> +        (JSC::MapData::clear):
> +        (JSC::MapData::MapData):
> +        * runtime/MapInstance.cpp: Added.
> +        (JSC::MapInstance::visitChildren):
> +        * runtime/MapInstance.h: Added.
> +        (JSC::MapInstance::createStructure):
> +        (JSC::MapInstance::create):
> +        (JSC::MapInstance::mapData):
> +        (JSC::MapInstance::MapInstance):

If you're not going to explain what you did, I don't think this listing is useful. So, please remove this listing, or explain what you did.

> Source/JavaScriptCore/runtime/MapConstructor.cpp:28
> +#include "config.h"
> +
> +#include "MapConstructor.h"

These lines should go together.

> Source/JavaScriptCore/runtime/MapConstructor.cpp:51
> +    // any arguments that could result in us throw.

"that could result in us throw"?

>> Source/JavaScriptCore/runtime/MapData.cpp:28
>> +#include "config.h"
>> +
>> +#include "JSCJSValueInlines.h"
> 
> Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]

These lines should go together.

>> Source/JavaScriptCore/runtime/MapData.cpp:30
>> +#include "MapData.h"
> 
> Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]

Ditto.

> Source/JavaScriptCore/runtime/MapData.cpp:139
> +template <MapData::CopyMode mode> void MapData::copyAndPackIfPossible(Entry* destination, size_t newSize)

I think this would be clearer as two different functions rather than one function with two templatized modes. Very little code here is in common.

> Source/JavaScriptCore/runtime/MapData.cpp:148
> +            memcpy(destination + newEnd, &entry, sizeof(Entry));

Let's use a copy constructor here instead.

> Source/JavaScriptCore/runtime/MapData.cpp:184
> +void MapData::visitChildren(JSCell* cell, SlotVisitor& visitor)

I don't think your test will trigger this function. Can you make it more aggressive, to cover this?

> Source/JavaScriptCore/runtime/MapData.h:106
> +        void next()
> +        {
> +            ASSERT(!atEnd());
> +            Entry* entries = m_mapData->m_entries;
> +            size_t index = m_index + 1;
> +            size_t end = m_mapData->m_size;
> +            while (index < end && !entries[index].key())
> +                index++;
> +            m_index = index;
> +        }

Can you add a test for iterating while inserting and removing?

> Source/JavaScriptCore/runtime/MapData.h:116
> +        ALWAYS_INLINE KeyType(JSValue v)
> +        {

It looks like this function tries to do some sort of double vs int conversion, but it's not clear what the goal is. I think you should factor out the conversion into a function, and name the function to indicate what we're trying to do.

> Source/JavaScriptCore/runtime/MapData.h:215
> +    HashMap<KeyType, size_t, KeyHash, KeyTraits, IndexTraits> m_valueKeyedTable;
> +    HashMap<StringImpl*, size_t> m_stringKeyedTable;

JavaScript never runs out of humor value.

> Source/JavaScriptCore/runtime/MapInstance.h:35
> +class MapInstance : public JSNonFinalObject {

Let's call this "JSMap" to match our other terminology..

[Darin Adler]

Comment on attachment 209944 [details]
Has full API spec and correctness

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

>> Source/JavaScriptCore/ChangeLog:45
>> +        (JSC::MapInstance::MapInstance):
> 
> If you're not going to explain what you did, I don't think this listing is useful. So, please remove this listing, or explain what you did.

I normally don’t include the list of newly added functions when I add a file even though the script generates the list. I haven’t heard a good argument against my practice.

[Oliver Hunt]

Created attachment 209998 [details]
Has full API spec and correctness

[WebKit Commit Bot]

Attachment 209998 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/fast/js/basic-map.html', u'LayoutTests/fast/js/script-tests/basic-map.js', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/heap/CopyToken.h', u'Source/JavaScriptCore/runtime/CommonIdentifiers.h', u'Source/JavaScriptCore/runtime/JSGlobalObject.cpp', u'Source/JavaScriptCore/runtime/JSGlobalObject.h', u'Source/JavaScriptCore/runtime/MapConstructor.cpp', u'Source/JavaScriptCore/runtime/MapConstructor.h', u'Source/JavaScriptCore/runtime/MapData.cpp', u'Source/JavaScriptCore/runtime/MapData.h', u'Source/JavaScriptCore/runtime/MapInstance.cpp', u'Source/JavaScriptCore/runtime/MapInstance.h', u'Source/JavaScriptCore/runtime/MapPrototype.cpp', u'Source/JavaScriptCore/runtime/MapPrototype.h', u'Source/JavaScriptCore/runtime/VM.cpp']" exit_code: 1
Source/JavaScriptCore/runtime/MapPrototype.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/runtime/MapData.cpp:28:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/JavaScriptCore/runtime/MapData.cpp:30:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/JavaScriptCore/runtime/MapInstance.cpp:28:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/JavaScriptCore/runtime/MapInstance.cpp:31:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/JavaScriptCore/runtime/MapInstance.cpp:40:  Missing space after ,  [whitespace/comma] [3]
Source/JavaScriptCore/runtime/MapConstructor.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/runtime/MapData.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/runtime/MapData.h:114:  Missing space inside { }.  [whitespace/braces] [5]
Source/JavaScriptCore/runtime/MapData.h:119:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/JavaScriptCore/runtime/MapData.h:158:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/MapData.h:158:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/MapData.h:159:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/MapData.h:160:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/MapData.h:205:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 15 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 209998 [details]
Has full API spec and correctness

Attachment 209998 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1643028

[Oliver Hunt]

(In reply to comment #36)
> (From update of attachment 209944 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209944&action=review
> 
> >> Source/JavaScriptCore/ChangeLog:45
> >> +        (JSC::MapInstance::MapInstance):
> > 
> > If you're not going to explain what you did, I don't think this listing is useful. So, please remove this listing, or explain what you did.
> 
> I normally don’t include the list of newly added functions when I add a file even though the script generates the list. I haven’t heard a good argument against my practice.

The changelog is just there so webkit-patch works automatically :D

[Early Warning System Bot]

Comment on attachment 209998 [details]
Has full API spec and correctness

Attachment 209998 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1635262

[Darin Adler]

Comment on attachment 209998 [details]
Has full API spec and correctness

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

>> Source/JavaScriptCore/runtime/MapData.cpp:30
>> +#include "MapData.h"
> 
> Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]

I agree with the style checker. This should not have to be included after the Inlines header.

> Source/JavaScriptCore/runtime/MapData.cpp:44
> +enum { MinimumMapSize = 16 };

Where does that constant come from? Why an enum instead of a const? This is C++, not C.

> Source/JavaScriptCore/runtime/MapData.cpp:58
> +    size_t location = 0;

This "= 0" is pointless and should be omitted.

> Source/JavaScriptCore/runtime/MapData.cpp:60
> +        String string = asString(key.value)->value(callFrame);

Not sure we need the local variable for this. Could do it all on one line.

> Source/JavaScriptCore/runtime/MapData.cpp:75
> +    return !find(callFrame, key).isEmpty();

isEmpty? Really? Not typically how iterators work. It would normally be == end(). This also seems like a function that would be inlined.

> Source/JavaScriptCore/runtime/MapData.cpp:87
> +    ASSERT(map.contains(key));

Seems a little strange to assert this. Do we really not trust map.add?

> Source/JavaScriptCore/runtime/MapData.cpp:97
> +    if (!location)
> +        return;

Usually we use == end, in contains we use isEmpty, and here we use ! -- would be better to be consistent.

> Source/JavaScriptCore/runtime/MapData.cpp:106
> +    String string = asString(key.value)->value(callFrame);

Not sure we need the local for this. Might be readable all on one line.

> Source/JavaScriptCore/runtime/MapData.cpp:119
> +    size_t location = 0;

Don't need the "=0" here.

> Source/JavaScriptCore/runtime/MapData.cpp:121
> +        String string = asString(key.value)->value(callFrame);

Not sure we need the local variable for this. Could do it all on one line.

> Source/JavaScriptCore/runtime/MapData.cpp:200
> +        visitor.appendValues(&entries[0].keyForGC(), size * (sizeof(Entry) / sizeof(WriteBarrier<Unknown>)));

Ugly to rely on the layout of the Entry in this subtle way here. If we are going to use it like this, I think it should be a struct and the restriction on its layout should be mentioned in the header. We should also have a compile-time assertion that there is no padding. The getters aren’t adding value if they just return references to same named data members.

> Source/JavaScriptCore/runtime/MapData.cpp:209
> +    if (token == MapBackingStoreCopyToken
> +        && visitor.checkIfShouldCopy(thisObject->m_entries)) {

Would read better on a single line.

> Source/JavaScriptCore/runtime/MapData.cpp:213
> +        void* newVector = visitor.allocateNewSpace(thisObject->m_capacity * sizeof(Entry));
> +        Entry* newEntries = static_cast<Entry*>(newVector);

newVector local is not doing us any good here

> Source/JavaScriptCore/runtime/MapData.h:37
> +namespace JSC {
> +class MapData : public JSDestructibleObject {

Missing blank line.

> Source/JavaScriptCore/runtime/MapData.h:38
> +private:

Should put public stuff first. Don’t have to define things before they are used.

> Source/JavaScriptCore/runtime/MapData.h:174
> +    void clear()
> +    {
> +        m_valueKeyedTable.clear();
> +        m_stringKeyedTable.clear();
> +        m_capacity = 0;
> +        m_size = 0;
> +        m_deletedCount = 0;
> +        m_entries = 0;
> +    }

All these functions inlined within the class make the class too hard to read. I suggest putting more of the non-trivial inline functions separate after the class so the class itself is more readable.

[Build Bot]

Comment on attachment 209998 [details]
Has full API spec and correctness

Attachment 209998 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1640222

[Build Bot]

Comment on attachment 209998 [details]
Has full API spec and correctness

Attachment 209998 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1626640

[EFL EWS Bot]

Comment on attachment 209998 [details]
Has full API spec and correctness

Attachment 209998 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1591953

[EFL EWS Bot]

Comment on attachment 209998 [details]
Has full API spec and correctness

Attachment 209998 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1626647

[kov's GTK+ EWS bot]

Comment on attachment 209998 [details]
Has full API spec and correctness

Attachment 209998 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1643030

[Build Bot]

Comment on attachment 209998 [details]
Has full API spec and correctness

Attachment 209998 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1639299

[Oliver Hunt]

Created attachment 210043 [details]
Patch

[WebKit Commit Bot]

Attachment 210043 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/js/basic-map-expected.txt', u'LayoutTests/fast/js/basic-map.html', u'LayoutTests/fast/js/script-tests/basic-map.js', u'Source/JavaScriptCore/CMakeLists.txt', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/GNUmakefile.list.am', u'Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj', u'Source/JavaScriptCore/JavaScriptCore.vcxproj/JavaScriptCore.vcxproj.filters', u'Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj', u'Source/JavaScriptCore/Target.pri', u'Source/JavaScriptCore/heap/CopyToken.h', u'Source/JavaScriptCore/runtime/CommonIdentifiers.h', u'Source/JavaScriptCore/runtime/JSGlobalObject.cpp', u'Source/JavaScriptCore/runtime/JSGlobalObject.h', u'Source/JavaScriptCore/runtime/JSMap.cpp', u'Source/JavaScriptCore/runtime/JSMap.h', u'Source/JavaScriptCore/runtime/MapConstructor.cpp', u'Source/JavaScriptCore/runtime/MapConstructor.h', u'Source/JavaScriptCore/runtime/MapData.cpp', u'Source/JavaScriptCore/runtime/MapData.h', u'Source/JavaScriptCore/runtime/MapPrototype.cpp', u'Source/JavaScriptCore/runtime/MapPrototype.h', u'Source/JavaScriptCore/runtime/VM.cpp']" exit_code: 1
Source/JavaScriptCore/runtime/MapData.h:173:  MapData::const_iterator::const_iterator is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 210043 [details]
Patch

Attachment 210043 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1623737

[Build Bot]

Comment on attachment 210043 [details]
Patch

Attachment 210043 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1639343

[Oliver Hunt]

Created attachment 210047 [details]
Patch

[Geoffrey Garen]

Comment on attachment 210047 [details]
Patch

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

r=me, with some more suggestions below.

> Source/JavaScriptCore/runtime/JSMap.cpp:28
> +#include "config.h"
> +
> +#include "JSMap.h"

No newline between these, please.

> Source/JavaScriptCore/runtime/MapData.cpp:28
> +#include "config.h"
> +
> +#include "MapData.h"

Ditto.

> Source/JavaScriptCore/runtime/MapData.cpp:43
> +static const int32_t MinimumMapSize = 8;

Should be lower case.

> Source/JavaScriptCore/runtime/MapData.cpp:68
> +bool MapData::contains(CallFrame* callFrame, KeyType key)

Newline here please.

> Source/JavaScriptCore/runtime/MapData.cpp:144
> +        entry.value.setWithoutWriteBarrier(jsNumber(newEnd));

I think it's worth a comment here to explain that you're being sneaky here. How about this:

"We store overwrite the old entry with a forwarding index for the new entry, so we can fix up our hash tables below without doing additional hash lookups."

> Source/JavaScriptCore/runtime/MapData.h:99
> +    // Our marking functions expect Entry to maintain this layout

You can be a little more specific and say "depend on all of our data members being WriteBarrier<Unknown>".

[Geoffrey Garen]

Comment on attachment 210047 [details]
Patch

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

> Source/JavaScriptCore/runtime/MapData.h:59
> +        // This is a bit gnarly. We use an index of -1 to indicate the
> +        // "end()" iterator. By casting to unsigned we can immediately
> +        // test if both iterators are at the end of their iteration.
> +        // We need this in order to keep the common case (eg. iter != end())
> +        // fast.
> +        bool atEnd() const { return static_cast<size_t>(m_index) >= static_cast<size_t>(m_mapData->m_size); }

I also think this is a bit gnarly. The typical solution, which WTF hash tables use, is to use m_mapData->m_size + 1 as the end index. That works naturally for ++, and avoids special-case checking for atEnd(). Would that work here?

[Oliver Hunt]

Committed r154861: <http://trac.webkit.org/changeset/154861>

[Oliver Hunt]

(In reply to comment #55)
> (From update of attachment 210047 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=210047&action=review
> 
> > Source/JavaScriptCore/runtime/MapData.h:59
> > +        // This is a bit gnarly. We use an index of -1 to indicate the
> > +        // "end()" iterator. By casting to unsigned we can immediately
> > +        // test if both iterators are at the end of their iteration.
> > +        // We need this in order to keep the common case (eg. iter != end())
> > +        // fast.
> > +        bool atEnd() const { return static_cast<size_t>(m_index) >= static_cast<size_t>(m_mapData->m_size); }
> 
> I also think this is a bit gnarly. The typical solution, which WTF hash tables use, is to use m_mapData->m_size + 1 as the end index. That works naturally for ++, and avoids special-case checking for atEnd(). Would that work here?

I considered that, but decided against it as it introduces a hazard if you use the idiom

foo = map->end();
for (ptr = begin(); ptr != end; ++ptr)

As Map iteration is specified as being safe under mutation, and has to visit newly added entries - any value that end() stores has to always be larger than size.  I guess we could just limit the map size to ~2gig - 1 entry and change the end marker to be <int>::max()