Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove useless setParent call and related condition. #4585

Merged
merged 1 commit into from
Jan 30, 2018

Conversation

rhuanjl
Copy link
Collaborator

@rhuanjl rhuanjl commented Jan 20, 2018

Follow up to issue: #3638

The issue was already fixed by removing the unnecessary assert:
#4548

But - it highlighted another small issue. The setParent call in JsrtCore is useless - it results in several operations all of which have no useful end result.

(No test case included as this doesn't fix a bug it just removes useless code)

@obastemur
Copy link
Collaborator

/cc @akroshg

@rhuanjl
Copy link
Collaborator Author

rhuanjl commented Jan 23, 2018

Small extra comment on this - additional to not doing anything useful this extra setParent command generated noise that made debugging some of the recently fixed module issues harder than it would otherwise have been.

@akroshg
Copy link
Contributor

akroshg commented Jan 24, 2018

This seems to be fine with me - although I'd ask @boingoing for the confirmation.

@rhuanjl
Copy link
Collaborator Author

rhuanjl commented Jan 25, 2018

@boingoing please could you take a look when you have a minute? This code is a rather annoying noise generator.

@boingoing
Copy link
Contributor

Sounds reasonable to me. Can we assert here that the child module has parent set to the referencing module?

@rhuanjl
Copy link
Collaborator Author

rhuanjl commented Jan 25, 2018

We should not assert here - it will fail. The child module record should never have a parent set at this point - this is the first of the two places it's currently set (and it's the wrong one.)

Steps in process:

  1. CC identifies that parent module has dependency and notifies host to fetch the child module:
    https://github.com/Microsoft/ChakraCore/blob/master/lib/Runtime/Language/SourceTextModuleRecord.cpp#L769

  2. (assuming module not already loaded) Host Initialises a module record with JsInitialiseModuleRecord which we're looking at here and returns it.

  3. CC then sets the parent:
    https://github.com/Microsoft/ChakraCore/blob/master/lib/Runtime/Language/SourceTextModuleRecord.cpp#L774-L779

Setting the parent in JsInitialiseModuleRecord is premature as it's about to be set within ResolveExternalModuleDependencies. Additionally as the dependency handling is all done with un-normalised specifiers whereas this set in JsInitialiseModuleRecord is done with a normalised specifier it therefore results in doubling up - as the two names may well not be the same.

@boingoing
Copy link
Contributor

Alright, I see what you mean. Seems fine to me.

@rhuanjl
Copy link
Collaborator Author

rhuanjl commented Jan 29, 2018

@boingoing let me know if this should be rebased to master and/or if you want me to do anything else with it.

@boingoing
Copy link
Contributor

This change should retarget master. 👍

@rhuanjl rhuanjl changed the base branch from release/1.8 to master January 29, 2018 22:11
@rhuanjl
Copy link
Collaborator Author

rhuanjl commented Jan 30, 2018

@boingoing I've rebased to master and updated. please let me know if you'd like me to do anything else with this.

@boingoing
Copy link
Contributor

I will do some internal testing and pull in the change. 👍

@chakrabot chakrabot merged commit 219fe44 into chakra-core:master Jan 30, 2018
chakrabot pushed a commit that referenced this pull request Jan 30, 2018
…ition.

Merge pull request #4585 from rhuanjl:setparent

Follow up to issue: #3638

The issue was already fixed by removing the unnecessary assert:
#4548

But - it highlighted another small issue. The setParent call in JsrtCore is useless - it results in several operations all of which have no useful end result.

(No test case included as this doesn't fix a bug it just removes useless code)
@rhuanjl rhuanjl deleted the setparent branch January 30, 2018 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants