Skip to content

Fix very slow jsdoc search in chained special assignments #23115

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

Merged
merged 1 commit into from
Apr 4, 2018

Conversation

sandersn
Copy link
Member

@sandersn sandersn commented Apr 3, 2018

Fixes #23111

The fix is to only recur once per parent instead of twice. This means moving the chained-assignment check in a slightly less obvious place.

@sandersn sandersn requested a review from a user April 3, 2018 17:03
@@ -1807,7 +1807,6 @@ namespace ts {
if (parent &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of checking if parent is defined over and over again, maybe you should bail out if parent isn't defined?

@sandersn
Copy link
Member Author

sandersn commented Apr 3, 2018 via email

@sandersn
Copy link
Member Author

sandersn commented Apr 3, 2018

@DanielRosenwasser here's what I got from de-duping the repeated parent checks. If you like it I can put it in a separate PR after this one:

if (parent) {
    if(parent.kind === SyntaxKind.PropertyAssignment ||
       parent.kind === SyntaxKind.PropertyDeclaration ||
       getNestedModuleDeclaration(parent) ||
       isBinaryExpression(node) && getSpecialPropertyAssignmentKind(node) ||
       isBinaryExpression(parent) && getSpecialPropertyAssignmentKind(parent) ||
       node.kind === SyntaxKind.PropertyAccessExpression && parent.kind === SyntaxKind.ExpressionStatement) {
        getJSDocCommentsAndTagsWorker(parent);
    }
    if (parent.parent) {
        // Try to recognize this pattern when node is initializer of variable declaration and JSDoc comments are on containing variable statement.
        // /**
        //   * @param {number} name
        //   * @returns {number}
        //   */
        // var x = function(name) { return name.length; }
        if (getSingleVariableOfVariableStatement(parent.parent) === node || getSourceOfAssignment(parent.parent)) {
            getJSDocCommentsAndTagsWorker(parent.parent);
        }
        if (parent.parent.parent) {
            if (getSingleInitializerOfVariableStatementOrPropertyDeclaration(parent.parent.parent) === node || getSourceOfDefaultedAssignment(parent.parent.parent)) {
                getJSDocCommentsAndTagsWorker(parent.parent.parent);
            }
        }
    }
}

Note that I removed the initial indent that comes from context, so the lines are actually longer than this in the editor.

@ghost
Copy link

ghost commented Apr 4, 2018

@sandersn Since this always ends in a call to getJSDocCommentsAndTagsWorker, maybe you could factor out a function getJSDocCommentsAndTagsHost that returns parent or parent.parent or parent.parent.parent or undefined.

@sandersn sandersn merged commit eb3a9d0 into master Apr 4, 2018
@sandersn sandersn deleted the fix-factorial-jsdoc-search branch April 4, 2018 15:57
@sandersn
Copy link
Member Author

sandersn commented Apr 4, 2018

@Andy-MS I tried this -- is it what you were proposing?

    function getJSDocCommentsAndTagsHost(node: Node) {
        return node.parent
            ? node.parent.parent
                ? node.parent.parent.parent ? node.parent.parent.parent : node.parent.parent
            : node.parent
        : undefined;
    }
        function getJSDocCommentsAndTagsWorker(node: Node): void {
            const host = getJSDocCommentsAndTagsHost(node);
            if (host &&
                (host.kind === SyntaxKind.PropertyAssignment ||
                 host.kind === SyntaxKind.PropertyDeclaration ||
                 getNestedModuleDeclaration(host) ||
                 getSingleVariableOfVariableStatement(host) === node ||
                 getSourceOfAssignment(host) ||
                 getSingleInitializerOfVariableStatementOrPropertyDeclaration(host) === node ||
                 getSourceOfDefaultedAssignment(host) ||
                 isBinaryExpression(node) && getSpecialPropertyAssignmentKind(node) !== SpecialPropertyAssignmentKind.None ||
                 isBinaryExpression(host) && getSpecialPropertyAssignmentKind(host) !== SpecialPropertyAssignmentKind.None ||
                 node.kind === SyntaxKind.PropertyAccessExpression && host.kind === SyntaxKind.ExpressionStatement)) {
                getJSDocCommentsAndTagsWorker(host);
            }
// ......

This doesn't work as-is, but might be close.

@ghost
Copy link

ghost commented Apr 4, 2018

Working on it.

@ghost
Copy link

ghost commented Apr 4, 2018

Never mind, the function needs to recurse on itself up to three times, my suggesstion would only work if it only needed to recurse once.

@microsoft microsoft locked and limited conversation to collaborators Jul 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

In JS, deeply chained exports assignments cause long compile time
2 participants