Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make a Node a factory for itself #199

Merged
merged 1 commit into from
Nov 5, 2017

Conversation

jvassev
Copy link
Contributor

@jvassev jvassev commented Nov 4, 2017

How about this: make the node class a factory for itself :) (details in the commit message)

Zero reflection on the fast path + 90KB classes less.

It is not how a real gentleman would structure their Java code but the circumstances are extreme :)

This patch refactors the Node to also be a
factory for self: this saves about 90K as
there are zero fields & field inits now and half
as much anonymous classes.

Jar size is reduce to just under 700K

NodeFactory is now an iterface with default methods
and factories are not singletons anymore.
@ben-manes
Copy link
Owner

I guess the only flaw is that this adds extra instance fields due to the factory node's unused ones? The alternatives would all have to add some, too, of course.

@jvassev
Copy link
Contributor Author

jvassev commented Nov 4, 2017

Not really, the factory is stateless. Take the generated WSo for example:

    ...
   // factory methods:
    public <K2, V2> Node<K2, V2> newNode(K2 key, ReferenceQueue<K2> keyReferenceQueue, V2 value,
        ReferenceQueue<V2> valueReferenceQueue, int weight, long now) {
      return new WSo<>(key, keyReferenceQueue, value, valueReferenceQueue, weight, now);
    }

    public <K2, V2> Node<K2, V2> newNode(Object keyReference, V2 value,
        ReferenceQueue<V2> valueReferenceQueue, int weight, long now) {
      return new WSo<>(keyReference, value, valueReferenceQueue, weight, now);
    }

    public <K2> Object newLookupKey(K2 key) {
      return new LookupKeyReference<K2>(key);
    }

    public <K2> Object newReferenceKey(K2 key, ReferenceQueue<K2> referenceQueue) {
      return new WeakKeyReference<K2>(key, referenceQueue);
    }
...

There are no instance fields in the factory as it is now an interface.

@ben-manes
Copy link
Owner

There are no instance fields in the factory as it is now an interface.

Sure there is. WSo has a key and value field.

@jvassev
Copy link
Contributor Author

jvassev commented Nov 4, 2017

Yes, the Wso used as factory will have some extra fields, but the Wso instances used as Node will have 4 more methods - no extra fields.

@ben-manes
Copy link
Owner

Yep, that's all I meant. It wasn't meant as more than an observation

@jvassev
Copy link
Contributor Author

jvassev commented Nov 4, 2017

Sorry, I thought you are concerned with bloating the Node footprint :)

@jvassev
Copy link
Contributor Author

jvassev commented Nov 4, 2017

Btw the K2, V2 are there to fix a few warnings. If we can live with the shadowing warnings that's 2K more saved and the psychological barrier of 700KB is passed :)

@ben-manes
Copy link
Owner

That would be okay or you could make NodeFactory typed. Then no shadowing would occur.

I'm not excited at the extra top-level fields, but I am also trying to see if there is any justification for that feeling. Its probably the right tradeoff.

@ben-manes
Copy link
Owner

When looking at the method handle approach, I guess it actually is worse because each MethodHandle holds a bunch of metadata fields. That prototype is at 680kb, with more work needed, but in comparison I think yours is nicer.

@jvassev
Copy link
Contributor Author

jvassev commented Nov 4, 2017

The enum constants would also create 144 field + 144 classes (in the metaspace) when probably 2-3 are really used. I would expect that memory usage is reduced a bit - assuming an app has a few caches now it would also need a create a few nodeFactories. The enum approach would always create 144 factories.

@ben-manes
Copy link
Owner

Oh I am definitely for removing the enum. It was just easy to write when trying to get the codegen working. There was a lot of upfront development costs to get this project going.

I can merge this unless you are still iterating on any of it?

@ben-manes ben-manes merged commit bda1898 into ben-manes:master Nov 5, 2017
@ben-manes
Copy link
Owner

I typed the NodeFactory and its now at 700kb. Thanks! :)

@jvassev jvassev deleted the node-factory-diet branch November 5, 2017 12:34
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.

2 participants