Add specialized types of Sets and Maps - #4816

MikeHolman · 2018-03-13T23:09:41Z

This change separates sets into three different types: Int, Simple, and Complex.

Int sets are sets which contain only int values. They are represented with a bit vector.
Simple sets contain values that are comparable by pointer alone, and use a normal dictionary.
Complex sets are sets that need full value comparison, and use a dictionary with a custom comparator.

I also added two kinds of maps, Simple and Complex, which function the same as simple and complex sets.

Improves perf of ARES-6 by 6% and VueJS by about 4%.

akroshg · 2018-03-13T23:16:47Z

+    if (setObject->kind != SetKind::EmptySet)
    {
-        return JavascriptOperators::GetTypeId(aValue) == TypeIds_Set;
+        JavascriptError::ThrowTypeErrorVar(scriptContext, JSERR_ObjectIsAlreadyInitialized, _u("Set"), _u("Set"));


assert (or telemetry assert) if that is the case? #Resolved

akroshg · 2018-03-13T23:23:37Z

+
+       if (!JavascriptNumber::TryGetInt32Value<true>(doubleVal, &intVal))
+       {
+           return nullptr;


nullptr [](start = 18, length = 7)

returning nullptr or false in this function seems to be inconsistent with return type. #Resolved

oops good point

In reply to: 174315122 [](ancestors = 174315122)

i meant for nullptr in the failure case (similar to what we do for the TryFromVar methods)

In reply to: 174315376 [](ancestors = 174315376,174315122)

akroshg · 2018-03-13T23:26:22Z

+        newIntSet->Set(intVal);

-        bool hasValue = set->Has(value);
+        this->list.Append(taggedInt, GetScriptContext()->GetRecycler());


GetScriptContext()->GetRecycler() [](start = 37, length = 33)

nit: you can use GetRecycler() here directly. #Resolved

akroshg · 2018-03-13T23:29:55Z

Curious : what is motivation behind the change ? cpu perf or memory ? #Resolved

MikeHolman · 2018-03-13T23:31:37Z

Motivation was CPU perf. I think there will be small wins in VueJS and ARES-6 due to faster map/set lookups (running perf tests now) #Resolved

akroshg · 2018-03-13T23:31:57Z

+    case SetKind::EmptySet:
+        return;
+    case SetKind::IntSet:
+        this->u.intSet->ClearAll();


Did we consider changing the kind to EmptySet ? #Pending

I considered it, but figured that often after clearing a set you will want to put things in it again, so it's cheaper to avoid extra allocation

In reply to: 174316471 [](ancestors = 174316471)

sounds good.

In reply to: 174319064 [](ancestors = 174319064,174316471)

akroshg · 2018-03-13T23:35:47Z

+    ScriptContext* scriptContext = function->GetScriptContext();

-        return scriptContext->GetLibrary()->GetUndefined();
+    JavascriptSet* set = JavascriptOperators::TryFromVar<JavascriptSet>(args[0]);


I think other functions below will require same love. :) #Resolved

akroshg · 2018-03-13T23:40:34Z

+            // Deleting any element will also cause the set to be promoted to a SimpleVarSet
+            IntSet,
+            // A SimpleVarSet is a set containing only Vars which are comparable by pointer, and don't require
+            // pointer comparison


value comparison ? #Resolved

akroshg · 2018-03-13T23:44:18Z


-    Var JavascriptSet::EntryHas(RecyclableObject* function, CallInfo callInfo, ...)
+    JavascriptSet::SetDataList::Iterator iter = this->list.GetIterator();
+    // TODO: we can use a more efficient Iterator, since we know there will be no side effects


this is a good point. Should we mark as JS_NO_REENTRANT region so that it is easy to explain that? #Resolved

I added lock to all of the general entrypoints

In reply to: 174318320 [](ancestors = 174318320)

agarwal-sandeep · 2018-03-14T00:06:49Z

// 3.    If Type(x) is Null, return true.

This says if type is undefined or null the return is true but you changed it to false below? #Pending

Refers to: lib/Runtime/Language/JavascriptConversion.cpp:44 in 985b16f. [](commit_id = 985b16f, deletion_comment = False)

agarwal-sandeep · 2018-03-14T00:23:58Z


-        JavascriptMap* map = JavascriptMap::FromVar(args[0]);
+bool
+JavascriptMap::DeleteFromSimpleVarMap(Var value)


Put assert that map kind is SimpleVarMap #Resolved

agarwal-sandeep · 2018-03-14T00:28:17Z


-        if (!JavascriptMap::Is(args[0]))
+bool
+JavascriptMap::Has(Var key)


For discussion: ContainsKey and TryGetValue only differ in terms of return.
Since the logic of Get and Has is same should we just use Get and return the result ignoring the value? #Pending

i tend to agree, but it's only a few lines of code and there might be some extra cost from those stores and bigger code might make inlining less aggressive... obviously small costs, but the code duplication win is also pretty small so i'm not sure it's worth it
if it gets any more complicated we should definitely merge these paths though

In reply to: 174324761 [](ancestors = 174324761)

agarwal-sandeep · 2018-03-14T00:32:38Z

-        return scriptContext->GetLibrary()->CreateMapIterator(map, JavascriptMapIteratorKind::Value);
+    JavascriptMap::MapDataList::Iterator iter = this->list.GetIterator();
+    // TODO: we can use a more efficient Iterator, since we know there will be no side effects
+    while (iter.Next())


Next [](start = 16, length = 4)

There is comment "Nodes can be deleted while iterating so validate current" in MapOrSetDataList.h Next() does that have any effect? #Pending

in general case, this iterator can be exposed to users, so elements might get deleted during iteration, but here that can't happen

In reply to: 174325329 [](ancestors = 174325329)

agarwal-sandeep · 2018-03-14T00:38:26Z


-        ARGUMENTS(args, callInfo);
-        ScriptContext* scriptContext = function->GetScriptContext();
+        newSimpleSet->Add(simpleVar, node);


I like the idea of adding to newIntSet fist and then to this->list as done in AddToEmptySet for case if Append throws then there is no inconsistency. #Pending

i thought the same. unfortunately i can't really do it here, because i need the node before adding to the set. i thought i might be able to add a dummy node and then update it, but seemed like might just be better to rely on our OOM failfast to maintain consistency

In reply to: 174326068 [](ancestors = 174326068)

agarwal-sandeep · 2018-03-14T00:40:00Z

+}

+void
+JavascriptSet::AddToEmptySet(Var value)


AddToEmptySet [](start = 15, length = 13)

nit: Can we put assert in all TryAddTo* for map kind? #Resolved

MikeHolman · 2018-03-14T00:44:55Z

// 3.    If Type(x) is Null, return true.

could be wrong, but I thought that if aLeft != aRight, then this equality check can't match

In reply to: 372860936 [](ancestors = 372860936)

Refers to: lib/Runtime/Language/JavascriptConversion.cpp:44 in 985b16f. [](commit_id = 985b16f, deletion_comment = False)

agarwal-sandeep · 2018-03-14T00:50:30Z

-            return set->Remove(value);
+            return false;
        }
+        this->PromoteToSimpleVarSet();


this->PromoteToSimpleVarSet(); [](start = 8, length = 30)

Why is a conversion required? #Resolved

it's hard to maintain list after deleting elements from an int set, because we don't have pointer to the element in the list. there might be some more complex work that can allow us to push the conversion until point where we need an iterator, but it seemed like pretty big complication, and scenario i'm looking at doesn't use delete (it just uses clear, which is fine).
I think this deserves a comment for now, and if it comes up, maybe we can improve this in future

In reply to: 174327605 [](ancestors = 174327605)

akroshg · 2018-03-14T17:10:38Z

        if (JavascriptOperators::IsUndefinedOrNullType(leftType))
        {
-            return leftType == rightType;
+            return false;


What happens if both values are undefined but belonged to different script context? #Pending

I would have thought they should be marshalled to correct script context by here? If that's not true, then Booleans are broken already in the existing code. #Pending

sethbrenith · 2018-03-15T16:54:41Z

+            // An EmptyMap is a map containing no elements
+            EmptyMap,
+            // A SimpleVarMap is a map containing only Vars which are comparable by pointer, and don't require
+            // pointer comparison


pointer comparison [](start = 15, length = 18)

value comparison #Resolved

sethbrenith · 2018-03-15T17:42:17Z

+        }
+        case TypeIds_String:
+        case TypeIds_Symbol:
+            // TODO: try to canonicalize Int64/Uint64 to TaggedInt


This TODO seems like a correctness issue, not an opportunity for future optimization, so I'm leaving a comment here to follow up. In particular, passing a 64-bit int to Has could incorrectly return false because it failed to canonicalize. #Resolved

Good point, I’ll add cases for these. #Closed

sethbrenith · 2018-03-15T17:58:39Z

+#endif
+        }
+        case TypeIds_String:
+        case TypeIds_Symbol:


TypeIds_Symbol [](start = 13, length = 14)

It looks like there's also an existing problem where SameValueComparer doesn't have any special case for Symbols and hashes them by reference. Would you like to fix that while you're working on related things, or should I open a separate bug? #Resolved

sure I can fix it. #Closed

MikeHolman · 2018-03-15T18:01:01Z

I realized that I have a bug with -0. I can’t canonicalize it to 0 when inserting into set/map, because when iterating over the set it’s expected that you get -0 out. (Same with Int64 types). #Resolved

sethbrenith

akroshg

Merge pull request #4816 from MikeHolman:setopt_pr This change separates sets into three different types: Int, Simple, and Complex. Int sets are sets which contain only int values. They are represented with a bit vector. Simple sets contain values that are comparable by pointer alone, and use a normal dictionary. Complex sets are sets that need full value comparison, and use a dictionary with a custom comparator. I also added two kinds of maps, Simple and Complex, which function the same as simple and complex sets. Improves perf of ARES-6 by 6% and VueJS by about 4%.

akroshg reviewed Mar 13, 2018

View reviewed changes

agarwal-sandeep reviewed Mar 14, 2018

View reviewed changes

akroshg reviewed Mar 14, 2018

View reviewed changes

MikeHolman force-pushed the setopt_pr branch from e798479 to 24d0906 Compare March 14, 2018 20:02

sethbrenith reviewed Mar 15, 2018

View reviewed changes

MikeHolman force-pushed the setopt_pr branch 5 times, most recently from 743368f to 87c16ba Compare March 15, 2018 21:52

sethbrenith approved these changes Mar 15, 2018

View reviewed changes

MikeHolman force-pushed the setopt_pr branch from 87c16ba to d1664ad Compare March 15, 2018 22:11

MikeHolman added 7 commits March 15, 2018 15:16

Add specialized types of Sets and Maps

fc4c320

address CR comments

73e66a3

fix xplat build

fba824a

address CR

c764ec5

fix assert

e6d81fd

fix SWB annotation for union

dde4b81

fix symbols; fix canonicalization of int64, -0

f9639b7

MikeHolman force-pushed the setopt_pr branch from d1664ad to f9639b7 Compare March 15, 2018 22:40

akroshg approved these changes Mar 16, 2018

View reviewed changes

chakrabot merged commit f9639b7 into chakra-core:master Mar 20, 2018

Uh oh!

Conversation

MikeHolman commented Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akroshg Mar 13, 2018 • edited by MikeHolman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akroshg Mar 13, 2018 • edited by MikeHolman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akroshg Mar 13, 2018 • edited by MikeHolman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akroshg commented Mar 13, 2018 • edited by MikeHolman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MikeHolman commented Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akroshg Mar 13, 2018 • edited by MikeHolman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akroshg Mar 13, 2018 • edited by MikeHolman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akroshg Mar 13, 2018 • edited by MikeHolman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akroshg Mar 13, 2018 • edited by MikeHolman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agarwal-sandeep commented Mar 14, 2018 • edited by MikeHolman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agarwal-sandeep Mar 14, 2018 • edited by MikeHolman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agarwal-sandeep Mar 14, 2018 • edited by MikeHolman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agarwal-sandeep Mar 14, 2018 • edited by MikeHolman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agarwal-sandeep Mar 14, 2018 • edited by MikeHolman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agarwal-sandeep Mar 14, 2018 • edited by MikeHolman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MikeHolman commented Mar 14, 2018

Uh oh!

MikeHolman commented Mar 13, 2018 •

edited

Loading

akroshg Mar 13, 2018 •

edited by MikeHolman

Loading

akroshg Mar 13, 2018 •

edited by MikeHolman

Loading

akroshg Mar 13, 2018 •

edited by MikeHolman

Loading

akroshg commented Mar 13, 2018 •

edited by MikeHolman

Loading

MikeHolman commented Mar 13, 2018 •

edited

Loading

akroshg Mar 13, 2018 •

edited by MikeHolman

Loading

akroshg Mar 13, 2018 •

edited by MikeHolman

Loading

akroshg Mar 13, 2018 •

edited by MikeHolman

Loading

akroshg Mar 13, 2018 •

edited by MikeHolman

Loading

agarwal-sandeep commented Mar 14, 2018 •

edited by MikeHolman

Loading

agarwal-sandeep Mar 14, 2018 •

edited by MikeHolman

Loading

agarwal-sandeep Mar 14, 2018 •

edited by MikeHolman

Loading

agarwal-sandeep Mar 14, 2018 •

edited by MikeHolman

Loading

agarwal-sandeep Mar 14, 2018 •

edited by MikeHolman

Loading

agarwal-sandeep Mar 14, 2018 •

edited by MikeHolman

Loading

agarwal-sandeep Mar 14, 2018 •

edited by MikeHolman

Loading

akroshg Mar 14, 2018 •

edited by MikeHolman

Loading

MikeHolman Mar 14, 2018 •

edited

Loading

sethbrenith Mar 15, 2018 •

edited by MikeHolman

Loading

sethbrenith Mar 15, 2018 •

edited by MikeHolman

Loading

MikeHolman Mar 15, 2018 •

edited

Loading

sethbrenith Mar 15, 2018 •

edited by MikeHolman

Loading

MikeHolman Mar 15, 2018 •

edited

Loading

MikeHolman commented Mar 15, 2018 •

edited

Loading