Skip to content

Conversation

aviroop-123
Copy link
Contributor

Enabling cached ZipFileSystemAccessor for Kotlin Native.

Closes: https://youtrack.jetbrains.com/issue/KT-79675/

@aviroop-123 aviroop-123 requested review from a team as code owners July 29, 2025 19:37
@ddolovov
Copy link
Contributor

ddolovov commented Aug 7, 2025

There are two "merge" commits which are undesirable. The single meaningful commit should be added as a result of rebasing on top of fresh master branch.

@ddolovov
Copy link
Contributor

ddolovov commented Aug 7, 2025

I would highly recommend to split the commit into two separate commits:

  1. The one that makes ZIP FS accessor available not only for JS but also for all KLIB-based compilers.
  2. The one that adds fine-tuning.

@ddolovov
Copy link
Contributor

ddolovov commented Aug 7, 2025

The commit message should contain a YT ticket number. It's better to put it onto a separate line. Example:

Some explanation.

^KT-00000


@Argument(
value = "-Xklib-zip-file-accessor-cache-limit",
description = "Size of cache to be used for the klib zip file accessor. Default is 0.",
Copy link
Contributor

Choose a reason for hiding this comment

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

The CLI key description does not specify the units in which the size is measured.

Also, it's unclear what does the default value ("0") exactly mean - disabled or unlimited?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the wordings, PTAL. I also think it should in general speed up the compiler, so I've setup the default value as 64. Please let me know if it makes sense.

@ddolovov
Copy link
Contributor

ddolovov commented Aug 7, 2025

I think, it worth adding KlibLoaderSpec.zipFileSystemAccessor() calls to:

andrey-mogilev
andrey-mogilev previously approved these changes Aug 15, 2025
Copy link
Collaborator

@andrey-mogilev andrey-mogilev left a comment

Choose a reason for hiding this comment

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

JVM-related changes looks good, thanks

@ddolovov
Copy link
Contributor

GH shows that there is a source conflict. Please rebase on fresh master, then I will run IC internally.

@lancelote lancelote removed their request for review August 28, 2025 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants