> On June 8, 2015, 3:47 p.m., Joel Hestness wrote: > > I like that this aims to remove some of the less common checkpoint writing > > functions. This is a useful step, and I'm largely ok with this change. > > > > I've always wondered why we don't explicitly pass Checkpoint to both > > serialize and unserialize functions. My only real comment is that if we're > > doing such substantial refactoring in that direction, why not make > > Checkpoint a more complete object that "contains" all the > > serialized/unserialized state? Specifically, it would make more sense to > > move the paramIn/paramOut functions inside Checkpoint. Details below. > > Andreas Sandberg wrote: > Hi Joel, > > Thanks for the review! I really want to move in this direction myself, > this patch is paving the way for a more ambitious rewrite of the > CheckpointIn/CheckpoinOut classes. The plan is that this change is the large > interface change that changes the signature of serialize/unserialize and that > the next change will be (mostly) kept to the checkpoint classes. I'm still > thinking about the interface though. There are a handful of features I'd like: > > * Transparent support for big binary blob (e.g., memory dumps, large > containers, frame buffers) serialization to separate files > * Move paramIn/paramOut to the checkpoint classes > * Transparent compression support > * Move static checkpoint state to a Checkpoint base class > (Checkpoint(In|Out) would inherit from this class) > > I'll be at the gem5 workshop if you want to discuss other ideas. > > Would you be OK with me submitting this as a an intermediate solution? > > Joel Hestness wrote: > Cool, I like these broader goals. > > I'd probably be fine with splitting these changes across patches as long > as the interface changes are committed close to each other. I would assume > most people will prefer to merge with these changes at most once, so I think > it would be most prudent to solidify the forward-looking interface either > within this patch or a successive patch. For instance, if the plan is to move > paramOut (paramIn) into the checkpoint class, then we should try to change > all existing calls from paramOut(os, ...) to cp.paramOut(...) here or very > closely following this patch.
I have only really given the future interface some limited thought, but I'm leaning towards a design where where the signatues of paramIn/paramOut won't need to change. The idea is that they would be analogous to the stream operators and be overridden by types that need to be serialized (e.g., Pixels). They would in turn be implemented using a low-level checkpoint interface (the CheckpointIn/CheckpointOut classes). The low-level interface would only need limited knowledge of the richer types implemented by paramIn/paramOut. In practice, we should only really need to support a very limited number of low-level types: * Unsigned integers (could be stored as uint64_t and bounds checked in paramIn/Out) * Signed integers (stored as int64_t and bounds checked in paramIn/Out) * Binary blobs * Strings * Vectors of the above This should be OK design wise and has the added benefit of not breaking existing code. If we decide that we want to move paramIn/Out into the checkpoint clases, we could still keep compatibility functions around that are flagged with M5_ATTR_DEPRECATED to give people some time to sort out their code. I'm not exactly a great fan of fixing up every single component in our internal repos either... > On June 8, 2015, 3:47 p.m., Joel Hestness wrote: > > src/sim/serialize.hh, line 69 > > <http://reviews.gem5.org/r/2861/diff/1/?file=45893#file45893line69> > > > > Could we move the ostream inside of Checkpoint and either (1) overload > > operator<<() there, or (2) better yet, move the paramIn/paramOut functions > > inside of Checkpoint? That way we could pass the same Checkpoint reference > > to all the serialize and unserialize functions, and we wouldn't have to > > distinguish these types (CheckpointIn vs. CheckpointOut). We'd have to > > modify the same code that this patch does, so changing this in a separate > > patch would require another broad merge. I looked into this at one point and there is quite a lot of state that ends up being input or output specific. I actually wanted to use just one class when I drafted my original design, but decided against because the input and output parts would be very different. The design I'm aiming for in the long run would be to have a BaseCheckpoint class that keeps track of common state (e.g., the active section), a CheckpointIn class that defines the interface for checkpoint loading, and CheckpointOut for the storage interface. This would be analogous to how istream/ostream inherit from the common ios base class. This would also mean that CheckpiontOut could be backed by an ostream and CheckpointIn by an istream. In the end, I decided that using the same class for both input and output would only complicate the logic inside the low-level checkpoint code without making any of the high-level stuff noticeably cleaner. Another benefit from keeping them separate is that copy-paste mistakes where unseriaize code appears in serialze() or vice versa would be detected by the compiler. - Andreas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2861/#review6474 ----------------------------------------------------------- On May 28, 2015, 6:35 p.m., Andreas Sandberg wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2861/ > ----------------------------------------------------------- > > (Updated May 28, 2015, 6:35 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10854:dfde63d4e52c > --------------------------- > sim: Refactor the serialization base class > > Objects that are can be serialized are supposed to inherit from the > Serializable class. This class is meant to provide a unified API for > such objects. However, so far it has mainly been used by SimObjects > due to some fundamental design limitations. This changeset redesigns > to the serialization interface to make it more generic and hide the > underlying checkpoint storage. Specifically: > > * Add a set of APIs to serialize into a subsection of the current > object. Previously, objects that needed this functionality would > use ad-hoc solutions using nameOut() and section name > generation. In the new world, an object that implements the > interface has the methods serializeSection() and > unserializeSection() that serialize into a named /subsection/ of > the current object. Calling serialize() serializes an object into > the current section. > > * Move the name() method from Serializable to SimObject as it is no > longer needed for serialization. The fully qualified section name > is generated by the main serialization code on the fly as objects > serialize sub-objects. > > * Add a scoped ScopedCheckpointSection helper class. Some objects > need to serialize data structures, that are not deriving from > Serializable, into subsections. Previously, this was done using > nameOut() and manual section name generation. To simplify this, > this changeset introduces a ScopedCheckpointSection() helper > class. When this class is instantiated, it adds a new /subsection/ > and subsequent serialization calls during the lifetime of this > helper class happen inside this section (or a subsection in case > of nested sections). > > * The serialize() call is now const which prevents accidental state > manipulation during serialization. Objects that rely on modifying > state can use the serializeOld() call instead. The default > implementation simply calls serialize(). Note: The old-style calls > need to be explicitly called using the > serializeOld()/serializeSectionOld() style APIs. These are used by > default when serializing SimObjects. > > * Both the input and output checkpoints now use their own named > types. This hides underlying checkpoint implementation from > objects that need checkpointing and makes it easier to change the > underlying checkpoint storage code. > > > Diffs > ----- > > src/arch/generic/types.hh e61f847e74fd > src/arch/mips/interrupts.hh e61f847e74fd > src/arch/mips/pagetable.hh e61f847e74fd > src/arch/arm/tlb.cc e61f847e74fd > src/arch/arm/types.hh e61f847e74fd > src/arch/arm/pmu.cc e61f847e74fd > src/arch/arm/tlb.hh e61f847e74fd > src/arch/arm/interrupts.hh e61f847e74fd > src/arch/arm/isa.hh e61f847e74fd > src/arch/arm/kvm/gic.hh PRE-CREATION > src/arch/arm/kvm/gic.cc PRE-CREATION > src/arch/arm/pagetable.hh e61f847e74fd > src/arch/arm/pmu.hh e61f847e74fd > src/arch/alpha/interrupts.hh e61f847e74fd > src/arch/alpha/isa.hh e61f847e74fd > src/arch/alpha/isa.cc e61f847e74fd > src/arch/alpha/kernel_stats.hh e61f847e74fd > src/arch/alpha/kernel_stats.cc e61f847e74fd > src/arch/alpha/pagetable.hh e61f847e74fd > src/arch/alpha/pagetable.cc e61f847e74fd > src/arch/alpha/process.hh e61f847e74fd > src/arch/alpha/process.cc e61f847e74fd > src/arch/alpha/system.hh e61f847e74fd > src/arch/alpha/system.cc e61f847e74fd > src/arch/alpha/tlb.hh e61f847e74fd > src/arch/alpha/tlb.cc e61f847e74fd > src/sim/voltage_domain.cc e61f847e74fd > src/sim/ticked_object.hh e61f847e74fd > src/sim/ticked_object.cc e61f847e74fd > src/sim/voltage_domain.hh e61f847e74fd > src/sim/system.hh e61f847e74fd > src/sim/system.cc e61f847e74fd > src/sim/sim_object.hh e61f847e74fd > src/sim/sim_object.cc e61f847e74fd > src/sim/root.hh e61f847e74fd > src/sim/root.cc e61f847e74fd > src/sim/serialize.hh e61f847e74fd > src/sim/serialize.cc e61f847e74fd > src/sim/sim_events.hh e61f847e74fd > src/sim/sim_events.cc e61f847e74fd > src/sim/cxx_manager.cc e61f847e74fd > src/sim/dvfs_handler.hh e61f847e74fd > src/sim/dvfs_handler.cc e61f847e74fd > src/sim/eventq.hh e61f847e74fd > src/sim/eventq.cc e61f847e74fd > src/sim/process.hh e61f847e74fd > src/sim/process.cc e61f847e74fd > src/python/swig/pyobject.cc e61f847e74fd > src/sim/clock_domain.hh e61f847e74fd > src/sim/clock_domain.cc e61f847e74fd > src/sim/cxx_manager.hh e61f847e74fd > src/mem/physical.cc e61f847e74fd > src/mem/ruby/system/System.hh e61f847e74fd > src/mem/ruby/system/System.cc e61f847e74fd > src/python/m5/SimObject.py e61f847e74fd > src/python/swig/core.i e61f847e74fd > src/python/swig/pyobject.hh e61f847e74fd > src/dev/arm/timer_cpulocal.hh e61f847e74fd > src/dev/arm/timer_cpulocal.cc e61f847e74fd > src/dev/arm/timer_sp804.hh e61f847e74fd > src/dev/arm/timer_sp804.cc e61f847e74fd > src/dev/arm/ufs_device.hh e61f847e74fd > src/dev/arm/ufs_device.cc e61f847e74fd > src/dev/arm/vgic.hh e61f847e74fd > src/dev/arm/vgic.cc e61f847e74fd > src/dev/copy_engine.hh e61f847e74fd > src/dev/copy_engine.cc e61f847e74fd > src/dev/copy_engine_defs.hh e61f847e74fd > src/dev/disk_image.hh e61f847e74fd > src/dev/disk_image.cc e61f847e74fd > src/dev/etherlink.hh e61f847e74fd > src/dev/etherlink.cc e61f847e74fd > src/dev/etherpkt.hh e61f847e74fd > src/dev/etherpkt.cc e61f847e74fd > src/dev/ethertap.hh e61f847e74fd > src/dev/ethertap.cc e61f847e74fd > src/dev/i2cbus.hh e61f847e74fd > src/dev/i2cbus.cc e61f847e74fd > src/dev/i8254xGBe.hh e61f847e74fd > src/dev/i8254xGBe.cc e61f847e74fd > src/dev/i8254xGBe_defs.hh e61f847e74fd > src/dev/ide_ctrl.hh e61f847e74fd > src/dev/ide_ctrl.cc e61f847e74fd > src/dev/ide_disk.hh e61f847e74fd > src/dev/ide_disk.cc e61f847e74fd > src/dev/intel_8254_timer.hh e61f847e74fd > src/dev/intel_8254_timer.cc e61f847e74fd > src/dev/mc146818.hh e61f847e74fd > src/dev/mc146818.cc e61f847e74fd > src/dev/mips/malta.hh e61f847e74fd > src/dev/mips/malta.cc e61f847e74fd > src/dev/mips/malta_cchip.hh e61f847e74fd > src/dev/mips/malta_cchip.cc e61f847e74fd > src/dev/mips/malta_io.hh e61f847e74fd > src/dev/mips/malta_io.cc e61f847e74fd > src/dev/mips/malta_pchip.hh e61f847e74fd > src/dev/mips/malta_pchip.cc e61f847e74fd > src/dev/ns_gige.hh e61f847e74fd > src/dev/ns_gige.cc e61f847e74fd > src/dev/pcidev.hh e61f847e74fd > src/dev/pcidev.cc e61f847e74fd > src/dev/pktfifo.hh e61f847e74fd > src/dev/pktfifo.cc e61f847e74fd > src/dev/sinic.hh e61f847e74fd > src/dev/sinic.cc e61f847e74fd > src/dev/sparc/dtod.hh e61f847e74fd > src/dev/sparc/dtod.cc e61f847e74fd > src/dev/sparc/iob.hh e61f847e74fd > src/dev/sparc/iob.cc e61f847e74fd > src/dev/sparc/mm_disk.hh e61f847e74fd > src/dev/sparc/mm_disk.cc e61f847e74fd > src/dev/uart8250.hh e61f847e74fd > src/dev/uart8250.cc e61f847e74fd > src/dev/virtio/base.hh e61f847e74fd > src/dev/virtio/base.cc e61f847e74fd > src/dev/virtio/fs9p.hh e61f847e74fd > src/dev/virtio/fs9p.cc e61f847e74fd > src/dev/x86/cmos.hh e61f847e74fd > src/dev/x86/cmos.cc e61f847e74fd > src/dev/x86/i8042.hh e61f847e74fd > src/dev/x86/i8042.cc e61f847e74fd > src/dev/x86/i82094aa.hh e61f847e74fd > src/dev/x86/i82094aa.cc e61f847e74fd > src/dev/x86/i8237.hh e61f847e74fd > src/dev/x86/i8237.cc e61f847e74fd > src/dev/x86/i8254.hh e61f847e74fd > src/dev/x86/i8254.cc e61f847e74fd > src/dev/x86/i8259.hh e61f847e74fd > src/dev/x86/i8259.cc e61f847e74fd > src/dev/x86/speaker.hh e61f847e74fd > src/dev/x86/speaker.cc e61f847e74fd > src/kern/kernel_stats.hh e61f847e74fd > src/kern/kernel_stats.cc e61f847e74fd > src/mem/cache/cache.hh e61f847e74fd > src/mem/cache/cache_impl.hh e61f847e74fd > src/mem/multi_level_page_table.hh e61f847e74fd > src/mem/multi_level_page_table_impl.hh e61f847e74fd > src/mem/page_table.hh e61f847e74fd > src/mem/page_table.cc e61f847e74fd > src/mem/physical.hh e61f847e74fd > src/dev/arm/rtc_pl031.hh e61f847e74fd > src/dev/arm/rtc_pl031.cc e61f847e74fd > src/dev/arm/rv_ctrl.hh e61f847e74fd > src/dev/arm/rv_ctrl.cc e61f847e74fd > src/cpu/testers/traffic_gen/traffic_gen.cc e61f847e74fd > src/cpu/thread_context.hh e61f847e74fd > src/cpu/thread_context.cc e61f847e74fd > src/cpu/thread_state.hh e61f847e74fd > src/cpu/thread_state.cc e61f847e74fd > src/dev/alpha/backdoor.hh e61f847e74fd > src/dev/alpha/backdoor.cc e61f847e74fd > src/dev/alpha/tsunami.hh e61f847e74fd > src/dev/alpha/tsunami.cc e61f847e74fd > src/dev/alpha/tsunami_cchip.hh e61f847e74fd > src/dev/alpha/tsunami_cchip.cc e61f847e74fd > src/dev/alpha/tsunami_io.hh e61f847e74fd > src/dev/alpha/tsunami_io.cc e61f847e74fd > src/dev/alpha/tsunami_pchip.hh e61f847e74fd > src/dev/alpha/tsunami_pchip.cc e61f847e74fd > src/dev/arm/energy_ctrl.hh e61f847e74fd > src/dev/arm/energy_ctrl.cc e61f847e74fd > src/dev/arm/flash_device.hh e61f847e74fd > src/dev/arm/flash_device.cc e61f847e74fd > src/dev/arm/generic_timer.hh e61f847e74fd > src/dev/arm/generic_timer.cc e61f847e74fd > src/dev/arm/gic_pl390.hh e61f847e74fd > src/dev/arm/gic_pl390.cc e61f847e74fd > src/dev/arm/hdlcd.hh e61f847e74fd > src/dev/arm/hdlcd.cc e61f847e74fd > src/dev/arm/kmi.hh e61f847e74fd > src/dev/arm/kmi.cc e61f847e74fd > src/dev/arm/pl011.hh e61f847e74fd > src/dev/arm/pl011.cc e61f847e74fd > src/dev/arm/pl111.hh e61f847e74fd > src/dev/arm/pl111.cc e61f847e74fd > src/cpu/minor/cpu.hh e61f847e74fd > src/cpu/minor/cpu.cc e61f847e74fd > src/cpu/o3/cpu.hh e61f847e74fd > src/cpu/o3/cpu.cc e61f847e74fd > src/cpu/o3/thread_state.hh e61f847e74fd > src/cpu/simple/base.hh e61f847e74fd > src/cpu/simple/base.cc e61f847e74fd > src/cpu/simple_thread.hh e61f847e74fd > src/cpu/simple_thread.cc e61f847e74fd > src/cpu/testers/traffic_gen/traffic_gen.hh e61f847e74fd > src/arch/x86/types.hh e61f847e74fd > src/arch/x86/types.cc e61f847e74fd > src/base/cp_annotate.hh e61f847e74fd > src/base/cp_annotate.cc e61f847e74fd > src/base/loader/symtab.hh e61f847e74fd > src/base/loader/symtab.cc e61f847e74fd > src/base/pollevent.hh e61f847e74fd > src/base/pollevent.cc e61f847e74fd > src/base/random.hh e61f847e74fd > src/base/random.cc e61f847e74fd > src/base/time.hh e61f847e74fd > src/base/time.cc e61f847e74fd > src/cpu/base.hh e61f847e74fd > src/cpu/base.cc e61f847e74fd > src/cpu/checker/cpu.hh e61f847e74fd > src/cpu/checker/cpu.cc e61f847e74fd > src/cpu/checker/thread_context.hh e61f847e74fd > src/cpu/kvm/BaseKvmCPU.py e61f847e74fd > src/cpu/kvm/base.hh e61f847e74fd > src/cpu/kvm/base.cc e61f847e74fd > src/cpu/kvm/x86_cpu.hh e61f847e74fd > src/cpu/kvm/x86_cpu.cc e61f847e74fd > src/arch/sparc/interrupts.hh e61f847e74fd > src/arch/sparc/isa.hh e61f847e74fd > src/arch/sparc/isa.cc e61f847e74fd > src/arch/sparc/pagetable.hh e61f847e74fd > src/arch/sparc/pagetable.cc e61f847e74fd > src/arch/sparc/system.hh e61f847e74fd > src/arch/sparc/system.cc e61f847e74fd > src/arch/sparc/tlb.hh e61f847e74fd > src/arch/sparc/tlb.cc e61f847e74fd > src/arch/x86/interrupts.hh e61f847e74fd > src/arch/x86/interrupts.cc e61f847e74fd > src/arch/x86/isa.hh e61f847e74fd > src/arch/x86/isa.cc e61f847e74fd > src/arch/x86/pagetable.hh e61f847e74fd > src/arch/x86/pagetable.cc e61f847e74fd > src/arch/x86/tlb.hh e61f847e74fd > src/arch/x86/tlb.cc e61f847e74fd > src/arch/mips/pagetable.cc e61f847e74fd > src/arch/mips/tlb.hh e61f847e74fd > src/arch/mips/tlb.cc e61f847e74fd > src/arch/power/pagetable.hh e61f847e74fd > src/arch/power/pagetable.cc e61f847e74fd > src/arch/power/tlb.hh e61f847e74fd > src/arch/power/tlb.cc e61f847e74fd > > Diff: http://reviews.gem5.org/r/2861/diff/ > > > Testing > ------- > > Quick regressions pass for all platforms, including checkpoint regressions on > ARM. Generated full-system checkpoint for X86, Alpha, and ARM. Compared > checkpoints before and after patch (checkpoints are identical). > > Note to reviewers: I'm really sorry about the size of this patch. :( > > > Thanks, > > Andreas Sandberg > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
