palantir / conjure-java

Conjure generator for Java clients and servers
Apache License 2.0
27 stars 43 forks source link

Implement optimization for Primitive lists #2274

Closed Shibi-bala closed 2 months ago

Shibi-bala commented 7 months ago

Before this PR

Conjure holds primitive arrays as boxed lists. Instead, we can switch to primitive array lists that are backed by primitive typed arrays (double[]) rather than lists (ArrayList).

After this PR

==COMMIT_MSG== ==COMMIT_MSG==

Possible downsides?

changelog-app[bot] commented 7 months ago

Generate changelog in changelog-dir>`changelog/@unreleased`</changelog-dir

What do the change types mean? - `feature`: A new feature of the service. - `improvement`: An incremental improvement in the functionality or operation of the service. - `fix`: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way. - `break`: Has the potential to break consumers of this service's API, inclusive of both Palantir services and external consumers of the service's API (e.g. customer-written software or integrations). - `deprecation`: Advertises the intention to remove service functionality without any change to the operation of the service itself. - `manualTask`: Requires the possibility of manual intervention (running a script, eyeballing configuration, performing database surgery, ...) at the time of upgrade for it to succeed. - `migration`: A fully automatic upgrade migration task with no engineer input required. _Note: only one type should be chosen._
How are new versions calculated? - ❗The `break` and `manual task` changelog types will result in a major release! - 🐛 The `fix` changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease. - ✨ All others will result in a minor version release.

Type

- [ ] Feature - [ ] Improvement - [ ] Fix - [ ] Break - [ ] Deprecation - [ ] Manual task - [ ] Migration

Description

Implement optimization for Primitive lists **Check the box to generate changelog(s)** - [ ] Generate changelog entry
carterkozak commented 7 months ago

In terms of sequencing, I think we should split this up into a few parts:

1) new generic list factory methods ConjureCollections which do not allow null values + update codegen to use them. note that list<any> should perhaps be allowed to contain null, as List.of(Optional.empty()) serializes to [null] which deserializes as an array containing null. 2) new specialized list factory methods for primitives in ConjureCollections.java, however these methods return the same types as in [1], but without generic inputs. Codegen can be updated to use these where possible 3) We can replace the types returned by the primitive-specialized methods with more efficient collection implementations without impacting behavior because we already have validation against nulls.

this sequencing is helpful because it enforces consistent non-nullness for all conjure collections, rather than based on specific types being used.

Shibi-bala commented 7 months ago

new generic list factory methods ConjureCollections which do not allow null values + update codegen to use them

@carterkozak Just to clarify this, should these new non-null ConjureCollections methods only be used if options.nonNullCollections is true. Otherwise, this is going to cause a break right?

carterkozak commented 7 months ago

That's correct