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

Should not sleep Fixes #356

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

Cyberboss
Copy link
Contributor

Fixes #355

Also splits SHOULD_NOT_SLEEP and SHOULD_BE_PURE tests and adds a minor improvement to the test helpers in that it shows more info where it wouldn't when panicing

@@ -562,7 +562,6 @@ pub struct AnalyzeObjectTree<'o> {
impure_procs: ViolatingProcs<'o>,
waitfor_procs: HashSet<ProcRef<'o>>,

sleeping_overrides: ViolatingOverrides<'o>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way this was implemented wasn't effective and the cause of the misses because it didn't catch overrides that called that called procs that slept.

@Cyberboss Cyberboss marked this pull request as draft May 6, 2023 22:32
/// Recursively visit this and all child **types**.
pub fn recurse_types<F: FnMut(TypeRef<'a>)>(&self, f: &mut F) {
self.recurse(f);
for parent_typed_idx in &self.tree.redirected_parent_types {
Copy link
Owner

Choose a reason for hiding this comment

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

this is going to visit things multiple times

Copy link
Contributor Author

@Cyberboss Cyberboss May 7, 2023

Choose a reason for hiding this comment

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

Is it? redirected_parent_types should only be populated with things that have different parent_types from their default path (i.e. setting parent_type = /mob on /mob/living is a no-op). So if recurse only hits direct child paths I don't see how it'd hit things multiple times.

@Cyberboss
Copy link
Contributor Author

As it stands, this is still too slow and uses way too much memory. More improvements incoming.

@Cyberboss Cyberboss marked this pull request as ready for review May 7, 2023 15:21
@Cyberboss
Copy link
Contributor Author

Cyberboss commented May 7, 2023

Okay @SpaceManiac This is latest on /tg/. The 7228 is the number of procs infected with SHOULD_NOT_SLEEP to begin with. Each print-out represents 10s of work.

7228 procs to anaylze call chains for
serialized objtree in 1.718s
11/7228 procs anaylzed
165/7228 procs anaylzed
449/7228 procs anaylzed
818/7228 procs anaylzed
1273/7228 procs anaylzed
1775/7228 procs anaylzed
2250/7228 procs anaylzed
2792/7228 procs anaylzed
3313/7228 procs anaylzed
3840/7228 procs anaylzed
4339/7228 procs anaylzed
4800/7228 procs anaylzed
5349/7228 procs anaylzed
6045/7228 procs anaylzed
6610/7228 procs anaylzed
7109/7228 procs anaylzed
dreamchecker 181.200s - total 192.546s

At this point the only optimization I can think of is to NEVER revisit procs (currently procs that don't have a sleep somewhere in their calls can get revisited) It would result in a slightly incomplete error set but it might be worth it.

@Cyberboss
Copy link
Contributor Author

I'm going to go ahead and do that

@Cyberboss Cyberboss marked this pull request as draft May 7, 2023 16:17
@Cyberboss
Copy link
Contributor Author

Back under 5s. It no longer prints out transitive errors but the time save is worth it.

@Cyberboss Cyberboss marked this pull request as ready for review May 7, 2023 16:52
@Cyberboss
Copy link
Contributor Author

There's a bug where the checking in /New() is too aggressive (We KNOW the type at that point and shouldn't be checking overrides).

@Cyberboss Cyberboss marked this pull request as ready for review May 7, 2023 20:48
@Cyberboss
Copy link
Contributor Author

Cyberboss commented May 7, 2023

/tg/ circa now dreamchecker 2.882s - total 18.013s ~136 errors

@Cyberboss
Copy link
Contributor Author

If anyone wants to take over this PR. It's basically done, except you need to get the commented out test from the above commit working

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.

Missed sleep with SHOULD_NOT_SLEEP
2 participants