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

Arch-split overhaul for design spec #809

Merged
merged 4 commits into from
Aug 19, 2024
Merged

Conversation

Xaphiosis
Copy link
Member

The adventure continues, this time deploying arch_requalify and arch_global_naming to the design spec skeleton files.
Previously @corlewis benchmarked #805 as having 13-18% wall time improvement for ASpec, and 9-14% improvement for AInvs. This time I'm expecting only a bit of improvement in ExecSpec/BaseRefine/CBaseRefine, since only 52 cases of context begin interpretation Arch . were eliminated from the design spec in total.

🦆🦆🦆 I have split up the commits to be topical, which should make reviewing this easier, but it's a bit too much granularity to shove into l4v. Opinions welcome on how many commits we want this squashed to.

  • still need to add FIXME arch_split to PR linter to stop people reintroducing it

@Xaphiosis Xaphiosis added enhancement cleanup proof engineering nicer, shorter, more maintainable etc proofs arch-split splitting proofs into generic and architecture dependent labels Aug 8, 2024
Copy link
Member

@corlewis corlewis left a comment

Choose a reason for hiding this comment

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

Very nice!

For your question about the commits, I actually don't see too much of a problem with how you've currently split it up. I would maybe combine the two 'use arch_global_naming' ones, although it's then not clear whether the arch_requalify part of the first one would belong there. Splitting up the generic and arch parts of deploying the architecture makes sense to me, since you need to do the generic ones first to easily see what can be removed from the arch files.

spec/design/skel/Interrupt_H.thy Show resolved Hide resolved
spec/design/skel/KernelInit_H.thy Outdated Show resolved Hide resolved
spec/design/skel/AARCH64/ArchStructures_H.thy Show resolved Hide resolved
spec/design/skel/ARM/State_H.thy Show resolved Hide resolved
@corlewis
Copy link
Member

Promising initial benchmark!

master:
  N = 10
  Average ARM ExecSpec: (00:01:34 real, 00:03:30 cpu, 4.78GB)
  Standard Deviation: (00:00:00 real, 00:00:01 cpu, 0.73GB)
arch-split_design:
  N = 10
  Average ARM ExecSpec: (00:01:20 real, 00:03:09 cpu, 4.65GB)
  Standard Deviation: (00:00:01 real, 00:00:03 cpu, 0.79GB)
Percentage change: -15.3%

I'll add the other architectures once the tests complete.

@corlewis
Copy link
Member

corlewis commented Aug 12, 2024

I'm almost a bit surprised with how good these numbers are!

master:
  N = 10
  Average ARM BaseRefine: (00:01:29 real, 00:02:36 cpu, 3.89GB)
  Standard Deviation: (00:00:01 real, 00:00:03 cpu, 0.37GB)
arch-split_design:
  N = 10
  Average ARM BaseRefine: (00:01:15 real, 00:02:16 cpu, 3.55GB)
  Standard Deviation: (00:00:01 real, 00:00:02 cpu, 0.51GB)
Percentage change: -15.6%
master:
  N = 10
  Average ARM_HYP BaseRefine: (00:02:07 real, 00:03:44 cpu, 4.25GB)
  Standard Deviation: (00:00:01 real, 00:00:07 cpu, 0.31GB)
arch-split_design:
  N = 10
  Average ARM_HYP BaseRefine: (00:01:41 real, 00:02:54 cpu, 4.74GB)
  Standard Deviation: (00:00:01 real, 00:00:02 cpu, 0.52GB)
Percentage change: -20.2%
master:
  N = 10
  Average X64 BaseRefine: (00:02:04 real, 00:03:34 cpu, 4.30GB)
  Standard Deviation: (00:00:01 real, 00:00:04 cpu, 0.45GB)
arch-split_design:
  N = 10
  Average X64 BaseRefine: (00:01:40 real, 00:02:56 cpu, 3.96GB)
  Standard Deviation: (00:00:02 real, 00:00:04 cpu, 0.59GB)
Percentage change: -19.5%
master:
  N = 10
  Average RISCV64 BaseRefine: (00:01:23 real, 00:02:28 cpu, 3.75GB)
  Standard Deviation: (00:00:01 real, 00:00:03 cpu, 0.67GB)
arch-split_design:
  N = 10
  Average RISCV64 BaseRefine: (00:01:07 real, 00:02:05 cpu, 3.59GB)
  Standard Deviation: (00:00:01 real, 00:00:03 cpu, 0.34GB)
Percentage change: -18.8%
master:
  N = 10
  Average AARCH64 BaseRefine: (00:01:56 real, 00:03:24 cpu, 4.44GB)
  Standard Deviation: (00:00:02 real, 00:00:05 cpu, 0.44GB)
arch-split_design:
  N = 10
  Average AARCH64 BaseRefine: (00:01:31 real, 00:02:42 cpu, 4.55GB)
  Standard Deviation: (00:00:01 real, 00:00:02 cpu, 0.89GB)
Percentage change: -22.0%

Copy link
Member

@lsf37 lsf37 left a comment

Choose a reason for hiding this comment

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

Looking good, good to see that almost all of the proof impact is for nicer names. And it looks like the infrastructure has everything we need so far. Nice work!

@lsf37
Copy link
Member

lsf37 commented Aug 12, 2024

In terms of commits, I wouldn't terribly mind the current state, but if you do want to squash more, I would squash the generic + arch part of the deployment, and maybe even put that together with the design: use arch_global_naming commit. If you want to go all-out even with the proof update for it. I'd keep the first two separate.

@corlewis
Copy link
Member

corlewis commented Aug 13, 2024

For completeness, CBaseRefine is already a fair bit quicker as well (master here is before all of your recent arch-split work, so the comparison includes the aspec, ainvs and design changes).

master:
  N = 10
  Average AARCH64 CBaseRefine: (00:19:44 real, 00:43:42 cpu, 19.60GB)
  Standard Deviation: (00:00:14 real, 00:01:02 cpu, 1.92GB)
arch-split_design:
  N = 10
  Average AARCH64 CBaseRefine: (00:16:51 real, 00:35:42 cpu, 18.39GB)
  Standard Deviation: (00:00:25 real, 00:02:12 cpu, 2.47GB)
Percentage change: -14.6%

These were requalified in the middle of the design spec and seemed out
of place.

Signed-off-by: Rafal Kolanski <[email protected]>
* makes global_naming implicit (based on L4V_ARCH)
* avoid unnecessary Arch context interpretation (slow)

Signed-off-by: Rafal Kolanski <[email protected]>
@corlewis
Copy link
Member

The latest changes look good to me, once squashed I think this is ready to be merged.

* make global_naming implicit (based on L4V_ARCH)
* avoid unnecessary Arch context interpretation (slow)
* change inclusion point of MachineExports to Types_H and eliminate
  duplicated requalifications
* migrate some Arch-theory requalifications to generic (more consistent
  interface)
* document some name clash disambiguations
* annotate vcpu type which can't be moved there, and leave vmrights
  which is needed for enum instantiations (can't be done in a locale)
* unify interface for initIRQController and maxIRQ by providing
  abbreviations in _H global_naming
* do not import generic constants sanitiseRegister and
  getSanitiseRegisterInfo; their definitions were never imported in lieu
  of using translated Arch versions that have the same type, causing
  aliasing warning during const requalification.
* update comments where requalify clobbers arch version from abstract
  spec.

Signed-off-by: Rafal Kolanski <[email protected]>
@Xaphiosis
Copy link
Member Author

Thank you again for benchmarking these @corlewis!

@lsf37
Copy link
Member

lsf37 commented Aug 19, 2024

Also happy with it, merging it now!

@lsf37 lsf37 merged commit e00f7b4 into seL4:master Aug 19, 2024
14 checks passed
@Xaphiosis Xaphiosis deleted the arch-split_design branch August 19, 2024 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-split splitting proofs into generic and architecture dependent cleanup enhancement proof engineering nicer, shorter, more maintainable etc proofs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants