Skip to content
This repository has been archived by the owner on Aug 19, 2020. It is now read-only.

Reduce time required to generate the project accessors #1199

Merged
merged 68 commits into from
Oct 23, 2018

Conversation

bamboo
Copy link
Member

@bamboo bamboo commented Oct 22, 2018

By emitting the relevant bytecode directly instead of calling the Kotlin compiler to do it.

The current one, based on the Kotlin compiler, and the challenger,
emitting the bytecode for the accessors directly.
This makes code generation faster by a factor of 100 which means it
now takes a few dozen milliseconds rather than a few seconds.
So the classpath for non-inlined accessors can be injected in the
script ClassLoaders.
They served their purpose and now are in the way.
For it makes the script classloading cache integration test fail due
to being too resource hungry.
So they are usable within the `configurations` block.
@JLLeitschuh
Copy link
Contributor

JLLeitschuh commented Oct 22, 2018

One of the concerns I have about adding this feature is that it significantly raises the barrier to entry to adding new generated code to the project as an outside contributor.

Any developer that doesn't know how to write JVM bytecode directly will be unable to contribute meaningful fixes or additions. Do you have any ways that this impact can be minimized?

Perhaps allowing new features to be added without a bytecode component and adding bytecode generation when it's found that the performance increase is necessary?

I just want to make sure the external contribution component of this change is considered.

@bamboo bamboo changed the title WIP: Reduce time required to generate the project accessors Reduce time required to generate the project accessors Oct 22, 2018
@bamboo bamboo requested a review from eskatos October 22, 2018 20:55
by limiting private members visibility

Signed-off-by: Paul Merlin <paul@gradle.com>
by limiting private members visibility

Signed-off-by: Paul Merlin <paul@gradle.com>
Copy link
Member

@eskatos eskatos left a comment

Choose a reason for hiding this comment

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

I think this is good to merge, left one request on some test. There are things to improve, I'd say the main ones are i/o handling, split of the byte code emitter in logical chunks to make it more easier to work with (eg. schema -> Accessors -> Fragments -> source & bytes), and, deduplication of some source generation (kotlinDslAccessorsReport). Please capture those and the ones I missed in an issue.

): T = dependency.also { action.execute(it) }


class AccessorBytecodeEmitterSpike : TestWithTempFiles() {
Copy link
Member

Choose a reason for hiding this comment

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

❌ rename to eg. KotlinMetadataTest?

@eskatos
Copy link
Member

eskatos commented Oct 22, 2018

@JLLeitschuh, I agree this makes contributing new accessors a bit harder. But I think it's not as bad as it sounds. A proposal for new accessors should start with some example source we all can copy and paste in our build scripts and try. The next step is to turn that into a source template we can generate, with its inputs. The only thing this PR changes is adding the bytecode generation.

Accessors should be lean, the complex things being done by the Gradle model. Their source should be short, so should their bytecode. We can collaborate on writing that short but tedious part.

DSL extensibility and collections elements are good because we can reuse them a lot, uniformly. For example, your PR to add accessors to extensions of DependencyHandler will not need any change to or addition of bytecode generation code.

We should also make the code simpler to work with. There's room for improvement, see my review comment above for some first steps.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants