youki original config

[utam0k]

Why?

We have been using Spec as a way to pass container information to other commands(e.g. start command). However, this is too much information and isn't good for performance. Therefore we'll introduce youki original config to store only the information we need only.

[utam0k]

@Furisto @yihuaf This pr is WIP now. However, what do you think of this?

[yihuaf]

I am not sure I follow the argument. Can you elaborate on why the spec "is too much information and isn't good for performance"?

[utam0k]

I am not sure I follow the argument. Can you elaborate on why the spec "is too much information and isn't good for performance"?

@yihuaf
Sure. A start and delete commands need the information about cgrop_path and hooks via the file(it is actually config.json)
Now, this file is the file copied spec file. There is much unnecessary information(e.g. devices, seccomp...) In the spec file when the start or delete command executes.
This PR prevents copying a large file (Spec) by only copying with necessary information when executing the start or delete commands when executing the create command.

[yihuaf]

That's an interesting observation. The concern would be if we have to add more things to the YoukiConfig later but I am in favor to try it out. It would also be interesting to benchmark the difference. To be honest, given the size of the spec file, which is likely to be less than 4K, I have a suspicion that the performance difference is small.

[utam0k]

@yihuaf I don't think it's a very big improvement about the performance, but the current form is one that I slacked off on in its initial form (I slacked off because I was so close to getting the sample working), so fixing that is a big part of it.

[Furisto]

I don't think it's a very big improvement about the performance, but the current form is one that I slacked off on in its initial form (I slacked off because I was so close to getting the sample working), so fixing that is a big part of it.

If performance is not the concern, where is the harm in saving the whole spec? With this addition we would have to maintain two spec "things" but the gain is not really clear to me. I am open to trying it out, but for me there would have to be a measurable performance improvement to justify this.

[utam0k]

@yihuaf @Furisto
I tried to measure the performance. What do you think of this? I was surprised that it was faster than I expected. I guess it's probably due to the reduced JSON parsing part.

$ $ hyperfine --prepare 'sudo sync; echo 3 | sudo tee /proc/sys/vm/drop_caches' --warmup 10 --min-runs 100 'sudo ./youki create -b tutorial a && sudo ./youki start a && sudo ./youki delete -f a'

Results

Target	Time (mean ± σ)	Range (min … max)
This PR (Release)	183.2 ms ± 52.5 ms	111.4 ms … 517.5 ms
Main Branch (Release)	227.2 ms ± 45.0 ms	64.2 ms … 972.6 ms
crun	159.2 ms ± 18.4 ms	106.4 ms … 210.6 ms

[Furisto]

Huh, proves again that you should always benchmark your performance improvements. I would not have expected such a big difference. Strange that the variance between runs is quite considerable.

[yihuaf]

Good thing we didn't just assume. May be this is also a good time we start doing some benchmark and profiling to understand deeper. That max of 972+ms definitely calls to be investigated. I would benchmark next just the serialization/de-serialization, and read/write of config.json separately. If that proves our thesis, then let's try this change. We may also want to put a comment down that the change is for performance reason. When in the future this struct becomes closer to the size of spec, we should switch back.

[codecov-commenter]

Codecov Report

Merging #447 (193f304) into main (2120d33) will increase coverage by 0.18%.
The diff coverage is 78.84%.

@@            Coverage Diff             @@
##             main     #447      +/-   ##
==========================================
+ Coverage   60.44%   60.63%   +0.18%     
==========================================
  Files          82       83       +1     
  Lines       12150    12180      +30     
==========================================
+ Hits         7344     7385      +41     
+ Misses       4806     4795      -11     

[YJDoc2]

Hey, this looks good 👍
Can you add @yihuaf 's suggestion :

We may also want to put a comment down that the change is for performance reason. When in the future this struct becomes closer to the size of spec, we should switch back.

Maybe add this in the doc-draft.md as well.

Also, do you think that the config.rs should be renamed to youki_config.rs or something, to avoid misunderstanding it as general runtime config or so?

[Furisto]

Looks good to me, apart from @YJDoc2 suggestion.

[utam0k]

@YJDoc2
Thanks for your comments :>

I commented on this area. I think the doc-draft is a bit much, but what do you think?
https://github.com/containers/youki/pull/447/files#diff-10c989310fea87b855588bfc33a18d95b4779d090fa372aecb10c01465be7c8aR15-R16

Hey, this looks good +1 Can you add @yihuaf 's suggestion :

We may also want to put a comment down that the change is for performance reason. When in the future this struct becomes closer to the size of spec, we should switch back.

Maybe add this in the doc-draft.md as well.

I renamed.

Also, do you think that the config.rs should be renamed to youki_config.rs or something, to avoid misunderstanding it as general runtime config or so?

[YJDoc2]

Sorry for the late response 😓 😓
I think the comments should be enough, doc-draft addition might not be needed. As @yihuaf and @Furisto have also approved the PR, I'll merge it now 👍