jsrt: JsObject Delete/Get/Has/OwnP../Set Property #3875

obastemur · 2017-10-04T20:11:37Z

AcmeAIR LTO gain (3%)

New JSRT property interface that API user can use without going through convert-to-property-id process.

See #3790 for details.

Also added a note;

TODO: Can we make PropertyString && LiteralStringWithPropertyStringPtr
share the string buffer?

Once this PR is merged, there will be an additional PR on node-chakracore end to benefit this new interface.

MSLaguana · 2017-10-04T21:41:43Z

lib/Common/Codex/Utf8Helper.h

-    ///
-    template <typename AllocatorFunction>
-    HRESULT NarrowStringToWide(_In_ AllocatorFunction allocator,_In_ LPCSTR sourceString, size_t sourceCount, _Out_ LPWSTR* destStringPtr, _Out_ size_t* destCount, size_t* allocateCount = nullptr)
+    inline HRESULT NarrowStringToWideNoAlloc(_In_ LPCSTR sourceString, size_t sourceCount, _Out_ LPWSTR destString, _Out_ size_t* destCount)


For the no-alloc case, you should pass in the size of destString so we can check for and avoid overflows.

MSLaguana · 2017-10-04T21:53:46Z

lib/Jsrt/Jsrt.cpp

+    AssertMsg(scriptContext->GetThreadContext()->IsScriptActive(), "Caller is expected to be under ContextAPIWrapper!");
+    PERFORM_JSRT_TTD_RECORD_ACTION(scriptContext, RecordJsRTGetProperty, propertyRecord, object);
+
+    *value = Js::JavascriptOperators::GetPropertyNoCache(object, propertyRecord->GetPropertyId(), scriptContext);


Why GetPropertyNoCache?

We know the type and we don't have cache info ptr. So, don't waste time checking the type again and doing cache related stuff.

MSLaguana · 2017-10-04T22:08:49Z

lib/Jsrt/Jsrt.cpp

@@ -1367,102 +1367,199 @@ CHAKRA_API JsPreventExtension(_In_ JsValueRef object)
    });
 }

+static JsErrorCode InternalGetPropertyRecord(Js::ScriptContext * scriptContext, JsValueRef key, _Out_ const Js::PropertyRecord ** propertyRecord)
+{


Is this deliberately weaker than being able to compute x[y], e.g. var y = 2, var x = [1,2,3]; x[y]? That is we deliberately do not support getting property records for numbers, null, true, false, undefined, or arbitrary objects which get converted to a string?

We have an interface for number JsGetIndexed... (also works similarly) for the time being.. Once / if we ever drop that interface, we may combine them here. I was designing in a way that they are handled in the same place. Then I realized, consumer may have a better chance to know property type hence no point wasting it here.

obastemur · 2017-10-05T14:01:07Z

@dotnet-bot test Ubuntu shared_ubuntu_linux_debug please

MSLaguana · 2017-10-05T16:35:08Z

lib/Runtime/Library/ConcatString.h

@@ -9,18 +9,30 @@ namespace Js
    class LiteralStringWithPropertyStringPtr : public LiteralString
    {
    private:
+        // TODO: Can we make PropertyString && LiteralStringWithPropertyStringPtr share the string buffer?
        PropertyString * propertyString;


@MikeHolman do you know why this isn't a Field? The CI seems to be complaining about it now, and I'm inclined to agree with it.

Yeah, it should be one.

MSLaguana · 2017-10-05T17:09:44Z

lib/Runtime/Language/JavascriptOperators.cpp

-                    propertyRecord = propertyString->GetPropertyRecord();
-                }
-
+                propertyString = str->GetPropertyStringLazy();


Why not also use GetPropertyStringLazy in OP_GetElementI?

? sorry not sure I understood.

Also looks like JavascriptConversion::ToPropertyKey could have some of it's code dealing with LiteralStringWithPropertyStringPtr replaced with a call to GetPropertyStringLazy

JavascriptConversion::ToPropertyKey and similar places good idea. Could you please clarify -> Why not also use GetPropertyStringLazy in OP_GetElementI ?

I was looking for where you had used GetPropertyStringLazy, and I saw some other places that GetPropertyString was used but you had not changed it to use GetPropertyStringLazy; is there a reason for that, or should we also update the other locations? Specifically for OP_GetElementI I was referring to https://github.com/Microsoft/ChakraCore/blob/master/lib/Runtime/Language/JavascriptOperators.cpp#L3697

MSLaguana · 2017-10-05T18:49:40Z

lib/Common/Codex/Utf8Helper.h

+        if (allocateCount != nullptr) *allocateCount = cbDestString;
+
+        *destStringPtr = destString;
+        HRESULT result = NarrowStringToWideNoAlloc(sourceString, sourceCount, destString, sourceCount, destCount);


I believe that the destBufferCount parameter here should be sourceCount+1 to allow for the null

obastemur · 2017-10-05T19:57:21Z

/cc @mrkmarron are you happy with the TTD changes?

/cc @liminzhu comments on new JSRT?

MikeHolman · 2017-10-05T20:09:10Z

lib/Runtime/Library/ConcatString.h


    public:
        PropertyString * GetPropertyString() const;
+        PropertyString * GetPropertyStringLazy(); // Get if it's there, otherwise bring it in.


nit: this name is confusing to me. i think we could come up with something better, maybe GetOrAddPropertyString?

I'm not strong on naming and what you suggest sounds good.

MikeHolman · 2017-10-05T20:12:47Z

lib/Runtime/Library/ConcatString.h

@@ -9,18 +9,30 @@ namespace Js
    class LiteralStringWithPropertyStringPtr : public LiteralString
    {
    private:
-        PropertyString * propertyString;
+        // TODO: Can we make PropertyString && LiteralStringWithPropertyStringPtr share the string buffer?


I think you can just overwrite the buffer on this once we populate the PropertyString field (and then PropertyString, LiteralStringWithPropertyStringPtr, and PropertyRecord will all be sharing same buffer)

Indeed, this is a trivial TODO. However, I would like to keep it separate. (That's why TODO) BTW; the issue is still open and it also mentions this improvement.

MikeHolman · 2017-10-05T20:22:41Z

lib/Runtime/Library/ConcatString.cpp

+        switch (charCount)
+        {
+            case 0:
+                return (LiteralStringWithPropertyStringPtr*) library->GetEmptyString();


Could you have GetEmptyString return LiteralStringWithPropertyStringPtr* so we can avoid this cast? otherwise someone could change the emptyString to some other type of string, and compiler won't complain

I could put an assert on the returning type (VT check)

Sure that would work, but I think it is strictly better to have more type information represented at compile time rather than relying on run time asserts.

Aren't we going to cast this type eventually? (library-GetEmptyString)

MikeHolman · 2017-10-05T21:46:13Z

Just FYI I have some changes in master that this will conflict with (refactoring, nothing major).

Also, any reason this is going in 1.7? Seems odd to be doing perf work in a release branch.

liminzhu · 2017-10-05T22:18:29Z

We should have an equivalent for JsHasOwnProperty for completeness. Also, do those APIs work with symbols?

obastemur · 2017-10-06T14:50:05Z

Also, any reason this is going in 1.7? Seems odd to be doing perf work in a release branch.

and stressful..

@digitalinfinity ?

mrkmarron · 2017-10-12T03:39:45Z

TTD parts look good to me. I am not sure if they have been run through the TTD top NPM tests yet which will test this API more thoroughly.

obastemur · 2017-10-30T15:50:08Z

self note for the PR;

~~JavascriptOperators::GetElementIHelper (etc.) could use PropertyString || LiteralStringWithPropertyStringPtr to identify the type.~~ done
~~JavascriptOperators::CheckPrototypesForAccessorOrNonWritableProperty~~ done

obastemur · 2017-11-01T01:49:55Z

Made some additional improvements. Will measure the perf separately but need review on this.

/cc @MikeHolman @MSLaguana

MSLaguana · 2017-11-03T00:22:45Z

lib/Common/Codex/Utf8Helper.h

            return E_OUTOFMEMORY;
        }

-        WCHAR* destString = (WCHAR*)allocator(cbDestString);
-        if (destString == nullptr)
+        if (destString == nullptr || sourceCount >= destBufferCount)


If destString == nullptr then we shouldn't be writing to it at all; perhaps put that check first?

Also isn't sourceCount >= destBufferCount legal? E.g. if the input narrow string has more 3-byte utf8 codepoints than 1-bye utf8 codeopints, the utf16 representation can be shorter than the input.

I didn't understand your first comment.

Second comment; -> sourceCount represents the length of char16 buffer. Allocator is supposed to allocate equal or more (prior to calculating exact correspondence). So we can convert using the fast path below.

This condition is saying if (destString == nullptr || ...) { destString[0] = 0} which will (try to) write to the null pointer. Also the previous condition will write to destString without checking whether it is null. We should either be checking if destString == nullptr up front and exiting with an error without writing to it, or we should be relying on SAL to make sure that we never pass in a null pointer I think.

Regarding the second point; Requiring that sourceCount >= destBufferCount is safe, but it may lead to excess memory usage. Prior to this PR the over-estimating allocations are (I believe) always temporary, and the string is copied to a more accurately sized buffer for long-term storage, or freed once the variable goes out of scope. The new usage that you're adding in recycler-allocates a (potentially) over-estimated amount and hands that off to to be stored in a LiteralString.

Perhaps it is worth being clear about what the contract of this function is? Are you supposed to supply a buffer with destBufferCount == sourceCount, and then the caller is responsible for adjusting to destCount when the call returns? Is the caller supposed to supply a buffer with size at least destCount which is initially unknown, and it is an error if destBufferCount < destCount?

Second part; Yes, the idea is/was, instead of double pass, allocate ASCII amount of memory. OR we will endup having double pass for all the strings.

MSLaguana · 2017-11-03T00:35:26Z

lib/Common/Codex/Utf8Helper.h

@@ -142,14 +134,44 @@ namespace utf8
            // instead of replacing them with the "replacement" chracter. Pass a flag to our
            // decoder to require such behavior
            utf8::DecodeUnitsIntoAndNullTerminateNoAdvance(remDestString, remSourceString, (LPCUTF8) sourceString + cbSourceString, DecodeOptions::doAllowInvalidWCHARs);


The assert just before here that cchDestString <= sourceCount should be cchDestString <= destBufferCount I believe (maybe strictly less to allow for the null terminator), and if this function is exposed outside of our code then I think it should be an if(...) { return E_OUTOFMEMORY;} and not an assert.

Not sure.. This PR didn't make any logical changes within the Utf8Helper stuff. Just split an existing interface for reuse purposes.

Added if (cchDestString > sourceCount)

MSLaguana · 2017-11-03T21:03:51Z

lib/Runtime/Language/JavascriptOperators.cpp

-        {
-            return CheckPrototypesForAccessorOrNonWritablePropertyCore<JavascriptString*, true, false>(instance, propertyNameString, setterValue, flags, info, scriptContext);
-        }
+        PropertyString * propertyString = nullptr;


This variable isn't used and that's causing some compile errors on windows.

obastemur · 2017-11-03T21:09:51Z

Made some additional changes, looks like FindPropertyRecord was also taking the slow path all the time. Will measure the perf separately.

MSLaguana · 2017-11-03T23:02:34Z

lib/Runtime/Library/JSON.cpp

@@ -431,10 +431,16 @@ namespace JSON

        if (value == nullptr)
        {
-            Js::PropertyString* propertyString = Js::PropertyString::TryFromVar(key);
-            if (propertyString)
+            Js::JavascriptString * propertyKey = key;


This is going to conflict with another JSON change that went in earlier today, might want to rebase your change and reapply if necessary

AcmeAIR LTO gain (3%) New JSRT property interface that API user can use without going through convert-to-property-id process. See # 3790 for details. Also added a TODO note; ``` TODO: Can we make PropertyString && LiteralStringWithPropertyStringPtr share the string buffer? ``` Once this PR is merged, there will be an additional PR on node-chakracore end to benefit this new interface.

…perty Merge pull request #3875 from obastemur:desert_cerodon AcmeAIR LTO gain (3%) New JSRT property interface that API user can use without going through convert-to-property-id process. See #3790 for details. Also added a note; ``` TODO: Can we make PropertyString && LiteralStringWithPropertyStringPtr share the string buffer? ``` Once this PR is merged, there will be an additional PR on node-chakracore end to benefit this new interface.

Merge pull request #4123 from obastemur:lesscmp See issue details #4110 - Fix unnecessary string comparison with PropertyRecord/Equal (acme-air hits often) - Improve BuiltInPropertyRecord string comparison (acme-air hits 3 times per request) This PR needs #3875 to be merged first. Results from micro-`test/benchmark` are flat.

obastemur requested review from digitalinfinity, MikeHolman, MSLaguana, mrkmarron and liminzhu October 4, 2017 20:11

obastemur force-pushed the desert_cerodon branch 4 times, most recently from 99be27c to e39729a Compare October 4, 2017 21:11

MSLaguana reviewed Oct 4, 2017

View reviewed changes

obastemur force-pushed the desert_cerodon branch 4 times, most recently from c8c22f6 to 3a9a41d Compare October 5, 2017 11:57

obastemur force-pushed the desert_cerodon branch from 3a9a41d to 72872dc Compare October 5, 2017 15:00

MSLaguana reviewed Oct 5, 2017

View reviewed changes

obastemur force-pushed the desert_cerodon branch from 72872dc to 547fd6f Compare October 5, 2017 19:04

MikeHolman reviewed Oct 5, 2017

View reviewed changes

obastemur force-pushed the desert_cerodon branch from 547fd6f to c803ffc Compare October 6, 2017 14:56

obastemur force-pushed the desert_cerodon branch 2 times, most recently from e651dc7 to fea3125 Compare November 1, 2017 01:27

obastemur force-pushed the desert_cerodon branch from fea3125 to 518c4f9 Compare November 1, 2017 01:51

This was referenced Nov 2, 2017

Equals: minor improvements #4123

Merged

Share types (i.e., allow PathTypeHandlers) for properties with non-standard attributes (non-writable, etc.). #4118

Merged

MSLaguana reviewed Nov 3, 2017

View reviewed changes

obastemur force-pushed the desert_cerodon branch 7 times, most recently from 79487c9 to cfaa57b Compare November 3, 2017 20:44

MSLaguana reviewed Nov 3, 2017

View reviewed changes

obastemur force-pushed the desert_cerodon branch from cfaa57b to 48dfd00 Compare November 3, 2017 21:08

obastemur force-pushed the desert_cerodon branch from 48dfd00 to 7caf634 Compare November 3, 2017 21:14

MSLaguana reviewed Nov 3, 2017

View reviewed changes

MSLaguana approved these changes Nov 3, 2017

View reviewed changes

obastemur added 2 commits November 6, 2017 19:11

fix native-shared python test file (non-CI use)

718f271

obastemur force-pushed the desert_cerodon branch from 7caf634 to fc6240e Compare November 6, 2017 19:47

chakrabot merged commit fc6240e into chakra-core:master Nov 6, 2017

obastemur mentioned this pull request Nov 7, 2017

chakrashim: Enable new Get/Has/Set etc. Property from String nodejs/node-chakracore#422

Open

jsrt: JsObject Delete/Get/Has/OwnP../Set Property #3875

jsrt: JsObject Delete/Get/Has/OwnP../Set Property #3875

Conversation

obastemur commented Oct 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obastemur Oct 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obastemur Oct 4, 2017 • edited Loading

Choose a reason for hiding this comment

obastemur commented Oct 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obastemur commented Oct 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeHolman commented Oct 5, 2017 • edited Loading

liminzhu commented Oct 5, 2017

obastemur commented Oct 6, 2017

mrkmarron commented Oct 12, 2017

obastemur commented Oct 30, 2017 • edited Loading

obastemur commented Nov 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obastemur commented Nov 3, 2017

Choose a reason for hiding this comment

obastemur Oct 4, 2017 •

edited

Loading

obastemur Oct 4, 2017 •

edited

Loading

MikeHolman commented Oct 5, 2017 •

edited

Loading

obastemur commented Oct 30, 2017 •

edited

Loading