Update PropertyRecord interface to preserve the record life time #4580

obastemur · 2018-01-19T19:41:06Z

No description provided.

MSLaguana · 2018-01-19T19:45:08Z

lib/Runtime/Library/ConcatString.cpp

@@ -143,7 +143,9 @@ namespace Js
        this->propertyString = propStr;
        if (propStr != nullptr)
        {
-            this->propertyRecord = propStr->GetPropertyRecord();
+            Js::PropertyRecord const * localPropertyRecord;
+            propStr->GetPropertyRecord(&localPropertyRecord);


Why the local variable here, rather than passing in &(this->propertyRecord)?

That wouldn't trigger SWB operator

Ah I see, makes sense. Could you please add a comment there so it doesn't get "cleaned up" later?

jackhorton

There is a lot of indirection that got removed in this PR. Is there any perf gain that you're aware of/expecting?

jackhorton · 2018-01-19T22:17:13Z

lib/Runtime/Library/ConcatString.h

@@ -13,7 +13,40 @@ namespace Js
        Field(const Js::PropertyRecord*) propertyRecord;

    public:
-        virtual Js::PropertyRecord const * GetPropertyRecord(bool dontLookupFromDictionary = false) override;
+        virtual void GetPropertyRecord(_Out_ PropertyRecord const** propRecord, bool dontLookupFromDictionary = false) override


Any reason why this moved to the header?

MSLaguana · 2018-01-19T22:37:13Z

lib/Runtime/Library/ConcatString.h

+            *propRecord = this->propertyRecord;
+        }
+
+        virtual Js::PropertyId GetPropertyId() override


Is this override needed if you've got the same code on Js::JavascriptString?

curtisman · 2018-01-19T23:26:59Z

lib/Runtime/Library/JavascriptString.cpp


-        return propertyRecord;
+        Assert(propertyRecord != nullptr);
+        return propertyRecord->GetPropertyId();


The propertyRecord could still be collected after this GetPropertyId() and propertyId becomes invalid, right?

For PropertyString?

obastemur · 2018-01-20T00:04:35Z

@jianchun please review.

jianchun · 2018-01-20T02:14:16Z

lib/Runtime/Types/SimpleDictionaryTypeHandler.cpp

        // WARNING: This will return false for PropertyStrings that are actually InternalPropertyIds
-        Assert(!PropertyString::Is(key) || !IsInternalPropertyId(((PropertyString*)key)->GetPropertyRecord()->GetPropertyId()));


!IsInternalPropertyId(((PropertyString*)key)->GetPropertyRecord()->GetPropertyId()) [](start = 43, length = 83)

nit: Since in this || branch key is PropertyString, can be simplified to !IsInternalPropertyId(((PropertyString*)key)->GetPropertyId()), keeping original one line ASSERT.

jianchun · 2018-01-20T02:20:32Z

lib/Runtime/Library/ConcatString.cpp

@@ -143,7 +143,10 @@ namespace Js
        this->propertyString = propStr;
        if (propStr != nullptr)
        {
-            this->propertyRecord = propStr->GetPropertyRecord();
+            // why use local variable ? read SWB wiki


nit: This comment isn't really helpful. No comment is ok -- any reader interested or doubtful could easily find out there is a type mismatch between "this->propertyRecord" vs. "->GetPropertyRecord(...).

obastemur · 2018-01-23T23:38:26Z

@MSLaguana @jackhorton @jianchun Thanks for the review!

…the record life time Merge pull request #4580 from obastemur:props_swb

… preserve the record life time Merge pull request #4580 from obastemur:props_swb

… interface to preserve the record life time Merge pull request #4580 from obastemur:props_swb

obastemur requested review from jianchun and curtisman January 19, 2018 19:41

MSLaguana reviewed Jan 19, 2018

View reviewed changes

jackhorton reviewed Jan 19, 2018

View reviewed changes

MSLaguana reviewed Jan 19, 2018

View reviewed changes

curtisman reviewed Jan 19, 2018

View reviewed changes

obastemur force-pushed the props_swb branch 4 times, most recently from 4f609f4 to 18f88ca Compare January 19, 2018 23:56

jianchun reviewed Jan 20, 2018

View reviewed changes

jianchun approved these changes Jan 20, 2018

View reviewed changes

Update PropertyRecord interface to preserve the record life time

3677469

obastemur force-pushed the props_swb branch from 18f88ca to 3677469 Compare January 23, 2018 22:54

chakrabot merged commit 3677469 into chakra-core:release/1.8 Jan 23, 2018

chakrabot pushed a commit that referenced this pull request Jan 23, 2018

[MERGE #4580 @obastemur] Update PropertyRecord interface to preserve …

e5e1c85

…the record life time Merge pull request #4580 from obastemur:props_swb

chakrabot pushed a commit that referenced this pull request Jan 24, 2018

[1.8>1.9] [MERGE #4580 @obastemur] Update PropertyRecord interface to…

4d96e78

… preserve the record life time Merge pull request #4580 from obastemur:props_swb

chakrabot pushed a commit that referenced this pull request Jan 24, 2018

[1.9>master] [1.8>1.9] [MERGE #4580 @obastemur] Update PropertyRecord…

5a98e4b

… interface to preserve the record life time Merge pull request #4580 from obastemur:props_swb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update PropertyRecord interface to preserve the record life time #4580

Update PropertyRecord interface to preserve the record life time #4580

obastemur commented Jan 19, 2018

MSLaguana Jan 19, 2018

obastemur Jan 19, 2018

MSLaguana Jan 19, 2018

jackhorton left a comment

jackhorton Jan 19, 2018

MSLaguana Jan 19, 2018

curtisman Jan 19, 2018

obastemur Jan 19, 2018

obastemur commented Jan 20, 2018

jianchun Jan 20, 2018 •

edited

Loading

jianchun Jan 20, 2018

obastemur commented Jan 23, 2018

		// WARNING: This will return false for PropertyStrings that are actually InternalPropertyIds
		Assert(!PropertyString::Is(key) \|\| !IsInternalPropertyId(((PropertyString*)key)->GetPropertyRecord()->GetPropertyId()));

Update PropertyRecord interface to preserve the record life time #4580

Update PropertyRecord interface to preserve the record life time #4580

Conversation

obastemur commented Jan 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackhorton left a 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 Jan 20, 2018

jianchun Jan 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obastemur commented Jan 23, 2018

jianchun Jan 20, 2018 •

edited

Loading