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

faster newInstance and setField via sun.misc.Unsafe #67

Merged
merged 11 commits into from
Apr 6, 2014

Conversation

dimzon
Copy link
Contributor

@dimzon dimzon commented Apr 5, 2014

enjoy

@aaberg
Copy link
Owner

aaberg commented Apr 5, 2014

Wow, this really looks promising. Unfortunately I can't get the tests to run!

I'm using Oracle java 1.7 on a Ubuntu box.

I think there is an error on line 24 of the UnsafeFieldSetterFactory. You should call field.getType instead of field.getClass, as that will just return the Field.Class instead of the type it represents.

But even after fixing this, many of the tests fail!

@dimzon
Copy link
Contributor Author

dimzon commented Apr 6, 2014

All tests all passed now. Found many awfull code (seems like You doesn't care about thread-safety)...

@dimzon
Copy link
Contributor Author

dimzon commented Apr 6, 2014

  • some optimiation
HandCoded took 661ms
Sql2oTypical took 725ms (9.68% slower)

@aaberg
Copy link
Owner

aaberg commented Apr 6, 2014

Tested this by selecting 1,000,000 rows from a H2 database and parsing into pojos. Your implementation seems to give a descent performance increase of about 10-15 %.

We need to do something about the FactoryFacade class. The current implementation sets the setter-factories in the static constructor, which makes it hard to create unit testing that tests both the Unsafe-based implementation and the reflection-based implementation. Maybe each instance of Sql2o should have its own FactoryFacade?

I can see what you mean about thread-safety in the PojoMetadata cache... That is not good! I will create a ticket about it and get it fixed. Did you find any other examples of non-threadsafe code?

@dimzon
Copy link
Contributor Author

dimzon commented Apr 6, 2014

We need to do something about the FactoryFacade class. The current implementation sets the setter-factories in the static constructor, which makes it hard to create unit testing that tests both the Unsafe-based implementation and the reflection-based implementation.

You doesn't need to unit-test whole DB->POJO sequence. Create unit-test against FieldSetterFactory interface and feed all known implementations to it. Same for ObjectConstructorFactory and MethodSetterFactory.

also current FactoryFacade is temporary solution, needs some refactoring.

Did you find any other examples of non-threadsafe code?

Yes, FutureDetector as example. Actually I believe You need strong refactoring wich breaks some public API. I have a serious expirience in ORM development, also with dealing with JDBC "quirks" etc. Also I found some algoritmic bottlenecks wich is hard to resolve without some refactoring.

@aaberg
Copy link
Owner

aaberg commented Apr 6, 2014

@dimzon You are of cause right about the unit-testing!

Could you take a look at pull request #69 ? I fixed the thread-safety issue for the PojoMetadata caching. I would really like your comment on it, if you don't mind?

@dimzon
Copy link
Contributor Author

dimzon commented Apr 6, 2014

commited abstract thread-safe cache implementation

@dimzon
Copy link
Contributor Author

dimzon commented Apr 6, 2014

@aaberg
need merge with Quirks - related refactorings from @aldenquimby before I can continue
please review code and merge

@aaberg aaberg merged commit e6eeedf into aaberg:master Apr 6, 2014
@aaberg
Copy link
Owner

aaberg commented Apr 6, 2014

Merged!

@aaberg aaberg added this to the 1.5.0 milestone May 5, 2014
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