Repository navigation
benchmarking/sweperf: honor --lifecycle-mode for suspend - #3068
Tim Hockin (thockin) merged 2 commits into
Conversation
Parnika Mandewalkar (pmandewalkar)
changed the title
|
PTAL when possible Haowei Cai (Roy) (@roycaihw) , Max Smythe (@maxsmythe), thank you!! |
…pend sweperf always sent a synchronous SuspendActor, ignoring the shared --lifecycle-mode flag that agentsession, ateenv and glutton honor. sweperf now embeds dynconfig.Lifecycle like those classes. With lifecycle_mode=pause it sends an asynchronous SuspendActor, recorded as SuspendActorAsync like the other classes. The default (suspend) keeps today's behavior. lifecycle_mode none and resume_mode implicit are refused, since every sweperf cycle ends in a suspend and resumes with ResumeActor. pause is the existing --lifecycle-mode value for async SuspendActor; PauseActor itself was removed in agent-substrate#2813.
dabd42b to
98a6b88
Compare
| err := u.tracedCall(ctx, "SuspendActor", func(callCtx context.Context, tr *metadata.MD) error { | ||
| name, mode := "SuspendActor", ateapipb.DurabilityMode_DURABILITY_MODE_SYNCHRONOUS | ||
| if dynconfig.Get[knobs](u.cfg.Dyn).LifecycleMode == dynconfig.LifecycleModePause { | ||
| name, mode = "SuspendActorAsync", ateapipb.DurabilityMode_DURABILITY_MODE_ASYNCHRONOUS |
There was a problem hiding this comment.
Should this just be enforced always? Is substrate still recommending sync mode? What are other tests doing? I thought the sync mode was going away.
Haowei Cai (Roy) (roycaihw)
left a comment
Haowei Cai (Roy) (roycaihw)
left a comment
There was a problem hiding this comment.
The default (suspend) keeps today's behavior, so existing runs are unchanged
Do we still care about sync behavior's performance? I thought it's more like an anti-pattern than a feature now.
|
Hmm good question! I modeled this after how all the other tests have implemented the flag (it's just adding the same porting for the most part that glutton and agent session have done). From this it seems like as of now sync isn't going away yet. The lifecycle-mode defaults to sync for all of the benchmark loads, so if we want to make sweperf default async, I'd like input from Max Smythe (@maxsmythe) and we can do that for all the test workloads? |
Haowei Cai (Roy) (roycaihw)
left a comment
Haowei Cai (Roy) (roycaihw)
left a comment
There was a problem hiding this comment.
I'm okay with this change assuming it's following the pattern with other tests. Maybe we should switch the default so the default is async instead of sync
Co-authored-by: Haowei Cai (Roy) <[email protected]>
|
Committed your suggestion! Discussed offline that if the sync implementation matches the other benchmarks it's okay. The async default question is still open and we can do that in another pr for all benchmarks if we want that. |
Haowei Cai (Roy) (roycaihw)
left a comment
Haowei Cai (Roy) (roycaihw)
left a comment
There was a problem hiding this comment.
LGTM
sweperf always sent a synchronous SuspendActor and ignored the shared
--lifecycle-modeflag that agentsession, ateenv and glutton honor. Lifecycle v2 defaults to async suspend, so sweperf couldn't measure the path most users take without a code change.sweperf now embeds
dynconfig.Lifecyclelike the other classes. With--lifecycle-modepause, it sends an asynchronous SuspendActor, recorded as SuspendActorAsync. The default (suspend) keeps today's behavior, so existing runs are unchanged.--lifecycle-modenone and--resume-modeimplicit are refused, since every sweperf cycle ends in a suspend and resumes with ResumeActor.note: pause is the existing
--lifecycle-modevalue for async SuspendActor according to how the other benchmarks do it; was wondering if we should change the naming for all since PauseActor is gone per #2813AI use: AI-assisted, I reviewed and tested it.