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

Limit WireLib.TriggerInput to avoid stack overflow #2457

Merged
merged 1 commit into from
Nov 4, 2022

Conversation

Vurv78
Copy link
Member

@Vurv78 Vurv78 commented Oct 20, 2022

Now, same as WireLib.TriggerOutput, WireLib.TriggerInput has a limit of being called 4 times a tick (for each input) to avoid stack overflows.

This fixes #2415

Now, same as WireLib.TriggerOutput, WireLib.TriggerInput has a limit of being called 4 times a tick to avoid stack overflows.

This fixes wiremod#2415
@thegrb93
Copy link
Contributor

Sounds like this could break some stuff. Why not just fix whatever's outputting too much.

@Vurv78
Copy link
Member Author

Vurv78 commented Oct 20, 2022

It didn't seem to break anything but ofc I didn't test any dupes or anything. I don't see how this would break stuff either, I don't think anything should be triggering the same input more than four times a tick.

But I have two other options:

  1. Restrict E2 to only be able to trigger itself once a tick
  2. Move the checks from TriggerInput to inside the postexecute hook itself / E2 internals (I didn't want to do this)

@brandonsturgeon
Copy link
Contributor

We could try this out on CFC for a week or so. We get a lot of E2 activity.

Would that be helpful?

@Vurv78
Copy link
Member Author

Vurv78 commented Nov 1, 2022

That would be nice, I think it's fine as it is right now, just need to make sure it doesn't break stuff like Sparky said (which I don't think it would, why would any wire components rely on being executed 4+ times in a single tick? Maybe digiscreens? But you can't even trigger outputs more than 4 times a tick so I dunno how inputs would be triggered faster intentionally.)

@brandonsturgeon
Copy link
Contributor

I think it looks good too. I'll use this branch on CFC for a few days and let you know if I see any errors that might be related 👍

@brandonsturgeon
Copy link
Contributor

brandonsturgeon commented Nov 4, 2022

We've been running this for about three days now. ~2,400 E2s were spawned in that time, and the only wiremod error I've seen is the holoemitter bug that I fixed in another PR. We've also received no bug reports since swapping over.

I'd say this PR is good to go 👍

@Vurv78
Copy link
Member Author

Vurv78 commented Nov 4, 2022

Yeah I was thinking it's fine, although the issue would likely be with niche wire components that might somehow rely on this behavior. Going to merge anyway, thank you

@Vurv78 Vurv78 merged commit 3cb58ac into wiremod:master Nov 4, 2022
@brandonsturgeon
Copy link
Contributor

Right - E2s was the only metric I could get, haha. I figure it's a decent measure of how much use wiremod generally saw during that period.

No problem

@Vurv78 Vurv78 deleted the input-limiting branch September 11, 2023 18:47
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.

E2 self input wirelink stack overflow
3 participants