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

Implements the concept of "mass concatenation" with a new, specialized ASTNode #10

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Altoids1
Copy link
Owner

@Altoids1 Altoids1 commented Feb 7, 2022

image

One common issue with most implementations of a concatenation operator is that it's left-associative and sequential, just like any typical operator. A .. B .. C .. D allocates into memory A, then AB, then ABC, then ABCD, on and on. For very long runs of successive concatenation, this can cause very large and unnecessary reallocation events for each iteration of the process.

This patch changes the behaviour of these to forgo the typical behaviour of binary operations in favour of a "single-batch" approach, where each operand is evaluated and appended to a master string, with the goal of making the number of allocations at least semi-linear with the number of operands.

I'm not wholly satisfied with this implementation, but it's a good step towards reducing the amount of redundant malloc/free calls currently littering the João project at the moment.

This PR made this script run twice as fast compared to the main branch:

/main(args)
{
    for(Value i = 0; i < 100_000; i+=1)
    {
        String str = "This index is called '" .. i .. ", and twice its value is " .. i * 2 .. "!";
        ##print(str);
    }
}

The goal of this is to minimize the amount of big-boy string copies are done for repetitive, mass concatenations. Calling resolve() now does at most N mallocs, instead of... N²/2 + 1 I think?
Previously it'd just assume that each operand will evaluate to something that'll fit in 16 bytes when cast to string.

This still does that with a supermajority of ASTNodes, but for specifically Literals, it bothers to be smarter, since the length of those are obviously known at parse-time.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant