Serialization

[GreyCat]

No description provided.

[GreyCat]

Let's try to avoid using var and imperative. foldLeft seems to do the trick here, cleanly conveying the semantics and allowing e.g. advanced parallelization of such loops:

Suggested change

	var wasUnaligned = false
	seq.foreach { (attr) =>
	val nowUnaligned = isUnalignedBits(attr.dataType)
	if (wasUnaligned && !nowUnaligned)
	lang.alignToByte(lang.normalIO)
	lang.attrWrite(attr, attr.id, defEndian)
	wasUnaligned = nowUnaligned
	}
	seq.foldLeft(false) { case (wasUnaligned, attr) =>
	val nowUnaligned = isUnalignedBits(attr.dataType)
	if (wasUnaligned && !nowUnaligned)
	lang.alignToByte(lang.normalIO)
	lang.attrWrite(attr, attr.id, defEndian)
	nowUnaligned
	}

[generalmimon]

Actually, one of the major changes in Java and Python runtime libraries related to serialization was that all alignToByte insertion logic is handled in the runtime libraries themselves and the alignToByte() call generation is disabled in the compiler for both Java and Python:

kaitai_struct_compiler/shared/src/main/scala/io/kaitai/struct/languages/JavaCompiler.scala

Lines 497 to 500 in 2eca3de

	// NOTE: the compiler does not need to output alignToByte() calls for Java anymore, since the byte
	// alignment is handled by the runtime library since commit
	// https://github.com/kaitai-io/kaitai_struct_java_runtime/commit/1bc75aa91199588a1cb12a5a1c672b80b66619ac
	override def alignToByte(io: String): Unit = {}

kaitai_struct_compiler/shared/src/main/scala/io/kaitai/struct/languages/PythonCompiler.scala

Lines 486 to 489 in 2eca3de

	// NOTE: the compiler does not need to output alignToByte() calls for Python anymore, since the byte
	// alignment is handled by the runtime library since commit
	// https://github.com/kaitai-io/kaitai_struct_python_runtime/commit/1cb84b84d358e1cdffe35845d1e6688bff923952
	override def alignToByte(io: String): Unit = {}

(and now I see a small mistake in the comment in PythonCompiler - technically it should be "the compiler does not need to output alignToByte() align_to_byte()" per Python naming)

---

So I guess the piece of code you're reviewing can be simplified to just lang.attrWrite(attr, attr.id, defEndian).

[Brachi]

Is there a limitation to make the insertion of pass conditional? Like in readHeader with isEmpty. For this particular function it doesn't look so straightforward given that attrs seems to always be non-empty.

Although innocuous, having a "dangling" pass stands out on first read and makes one wonder if there was some kind of bug in the code generation or in the source ksy file. Probably a nit though!

def _fetch_instances(self):
    pass
    self.attr_01._fetch_instances()
    ...

Same occurrences in:

• checkHeader
• checkInstanceHeader
• condIfHeader
• condRepeatCommonHeader
• switchIfCaseFirstStart
• switchIfCaseStart
• switchIfElseStart

[generalmimon]

Is there a limitation to make the insertion of pass conditional? Like in readHeader with isEmpty. For this particular function it doesn't look so straightforward

Yep, it's not very straightforward with how the current compiler code is internally structured. The problem is that when the compiler generates a method header (typically in the *Header methods), it doesn't know whether that function will end up having some body or not. In case of readHeader, we're just very lucky that the body will be empty if and only if there are seq fields. Things get a bit more complicated in case of writeHeader, but it's doable - generated _write methods might not be empty even if there are no seq fields, provided there are parse instances that need their "write itself upon access" flags initialized:

kaitai_struct_compiler/shared/src/main/scala/io/kaitai/struct/ClassCompiler.scala

Line 333 in cb0c1eb

lang.writeHeader(defEndian, !instances.values.exists(i => i.isInstanceOf[ParseInstanceSpec]) && seq.isEmpty)

For _check or _fetchInstances (let alone something reused for many different purposes like condIfHeader!), trying to predict ahead of time with an ultimate boolean expression whether the function will or won't be empty borders on insanity (again, with the current code structure). It's of course theoretically possible, just absolutely unmaintainable and definitely not worth spending time on.

I'll rather take all the superfluous passes - the worst thing that can happen is that someone inspects the generated code and the passes "stand out" to them, as you put it. Far worse would be if we tried to eliminate them, but in that effort we forget to insert a pass in some cases when it should be there, which means the entire generated Python code will be rejected by Python interpreter and people will actually have to patch these manually because KS compiler produced invalid code.

So until there's time and willingness to refactor the compiler code so that we can reliably know whether the function body is empty or not at the time of generating its header, I don't think there's any point in doing anything about this. And this is very low priority for me, given that it only affects code purity. Almost every KS issue should have a higher priority than this.

[Brachi]

Thanks a lot for the long and insightful answer! This confirms my suspicion that it was tricky. I agree this is very low priority, sorry for the noise[1].
Thanks also for the tremendous effort done in serialization. I'm super excited about this feature :D

[1] At the risk of adding more noise, just in case I wanted to point out that an empty docstring is an alternative to using pass for an empty body function. But probably without content it's still not worth it.

[generalmimon]

For completeness: I've realized it wouldn't be hard to only insert passes when they're needed if we added pass in the *Footer method instead of the *Header method. At the time of when *Footer is called, we already know whether there was some code inserted since the corresponding *Header.

I may explore this idea one day.

[generalmimon]

@GreyCat Could you please continue with the review?

[dakhnod]

@GreyCat @generalmimon Hey, how is this getting along? Would be great to have serialization support in main!