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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: Support dependent strings for concatenation #16308

Merged
merged 2 commits into from
May 26, 2024

Conversation

adrian17
Copy link
Collaborator

@adrian17 adrian17 commented May 13, 2024

This makes affects all code that uses AvmString::concat, but the most important case is "a" + "b" in both AVM1 and AVM2.
(also added it to AS3 String::concat while I was there)

With the PR, the code like

var a = "";
for (var i = 0; i < N; i += 1) { a += "x" };

Will allocate a new string buffer log(N) times, instead of N times. (note that it'll still allocate N string objects) This makes most code like this super fast instead of... crashing the player.

Should fix several SWFs, including #9736 - though I didn't test it yet.

I still want to test the code a bit more (tbh adding test-only code to inspect string internals would be great 馃槩) and mostly get a safety review from @moulins if possible.
Also might still refactor it a bit, this entire code might be moved to AvmStringRepr to keep AvmString unsafe-free and avoid exposing internal pointers.

@adrian17
Copy link
Collaborator Author

adrian17 commented May 13, 2024

Forgot to add: for code that is not egregiously written (so most code), this might slightly increase our memory usage.
Since if every string concat is only ever done once, like data["c"+i]=1, then the overallocation is never useful.
That said, strings are usually not a huge % of total memory used, so it shouldn't make a noticeable difference on average.

@MartySVK
Copy link
Contributor

#9736 is fixed with this PR 馃コ

@torokati44 torokati44 force-pushed the avm2-dependent-string-concat branch from 897bab3 to 7f83143 Compare May 17, 2024 06:55
@torokati44
Copy link
Member

torokati44 commented May 17, 2024

(After rebasing, I have amended this commit with a cargo fmt and a tiny clippy lint fix, just to get rid of the red CI crosses, I hope you don't mind...)

@torokati44 torokati44 force-pushed the avm2-dependent-string-concat branch from 7f83143 to 6fdfe7d Compare May 17, 2024 07:03
@danielhjacobs danielhjacobs added the waiting-on-review Waiting on review from a Ruffle team member label May 19, 2024
@adrian17 adrian17 force-pushed the avm2-dependent-string-concat branch 4 times, most recently from f48f244 to 1079695 Compare May 23, 2024 22:20
@@ -38,6 +38,13 @@ impl<'gc> AvmString<'gc> {
}
}

pub fn as_managed(self) -> Option<Gc<'gc, AvmStringRepr<'gc>>> {
Copy link
Member

Choose a reason for hiding this comment

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

Do we use the 'managed' terminology anywhere else? If not, could we call this as_owned() ?

Copy link
Collaborator Author

@adrian17 adrian17 May 23, 2024

Choose a reason for hiding this comment

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

Oh sorry, forgot to mention (I also intended to add a TODO comment).
Currently the Owned naming is extremely misleading, since a Source::Owned string can either be dependent ("owned") or non-dependent ("owner"). IMO to resolve the overloaded usage of the term, the Source::Owned should become Source::Managed (in the "gc-managed" sense - feel free to propose alternatives), and "owner" terminology will only relate to dependent strings.

I didn't want to refactor this naming in this PR, but I did "pave the way" with .as_managed, since otherwise having left.owner().as_owned() sounds like utter nonsense.

@adrian17
Copy link
Collaborator Author

Note: the PR is still missing size overflow checks, but the pointer logic should be stable now.

@adrian17 adrian17 force-pushed the avm2-dependent-string-concat branch 3 times, most recently from 30c4918 to 40bdff9 Compare May 26, 2024 09:46
@adrian17 adrian17 force-pushed the avm2-dependent-string-concat branch from 40bdff9 to fc773ed Compare May 26, 2024 09:46
@adrian17 adrian17 merged commit bafc5d2 into ruffle-rs:master May 26, 2024
17 checks passed
@adrian17 adrian17 deleted the avm2-dependent-string-concat branch May 26, 2024 10:14
@danielhjacobs danielhjacobs removed the waiting-on-review Waiting on review from a Ruffle team member label May 26, 2024
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.

None yet

6 participants