search

eternagame / EternaJS

Eterna game/RNA design interface
Other
12 stars 10 forks source link

optim-reduce-fullsequence-cloning #552

Open aminere opened 3 years ago

aminere commented 3 years ago

Avoid calling this._sequence.slice(0) while iterating over bases

luxaritas commented 3 years ago

Keep in mind there are a couple places this is done because the sequence gets mutated, but the "caller" (whoever is passing the sequence) expects it not to be mutated. For example the sequence in an UndoBlock or Solution should never change, but Pose2D has a sequence that can be mutated internally, so it must be copied in order to preserve the correct behavior. (This has led to a class of bugs in the past)

everyday847 commented 3 years ago

Yes, this is deliberate sometimes. It's a defensive choice, so you will have to extensively debug each place where you elect to make a change!

On Tue, Jun 8, 2021, 3:18 PM Jonathan Romano @.***> wrote:

Keep in mind there are a couple places this is done because the sequence gets mutated, but the "caller" (whoever is passing the sequence) expects it not to be mutated. For example the sequence in an UndoBlock or Solution should never change, but Pose2D has a sequence that can be mutated internally, so it must be copied in order to preserve the correct behavior. (This has led to a class of bugs in the past)

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/eternagame/EternaJS/issues/552#issuecomment-857215636, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGGY2ZQ2HS4VE2DMOFEUPLTR2JMBANCNFSM46K2ZJGQ .

aminere commented 3 years ago

Roger that @luxaritas and @everyday847 ! I think this should be safe as it only gets rid of cloning while iterating over the sequence Here is the PR: https://github.com/eternagame/EternaJS/pull/553 If anything let me know!

luxaritas commented 3 years ago

That change definitely looks sensible - if we're actually slicing that many times that certainly looks quite expensive!