Refactor instrumentation

[ericbn]

It seems like there's no way out of it, if we want to keep static methods in Model class! ;- (

And moving those methods from the Model to another class, as non-static ones, would take all the fun out of AJ! :- )

[ericbn]

What's your opinion about going from

Person.where("name = ?", "John");

to something like

new SelectFrom(Person.class).where("name = ?", "John");

?

[ipolevoy]

yeah, I was thinking along the terms:

s(Person.class).where(...)
e(Person.class).delete(...)
e(Person.class).update(...)

where 's' is for selects, 'e' is for delete/update/insert. It is wairly easy to implement this, and people will be able to use AJ without instrumentation. The reason this is not done yet, is because even this is quite ugly.

[ericbn]

Great! I'm having new ideas about this, and we're still on the same page!

I'm having this idea we can offer both options for our users: models without instrumentation (they must provide the class, as above), or models with instrumentation (they don't need to worry about the above, but need the extra step after compilation). So they can choose which one bothers them less! ;- )

I'm starting to write this with the suggestions: https://github.com/ericbn/activejdbc/wiki

[ericbn]

The instrumentation option is provided with an extra dependency users must add to their pom.xml, that adds an extra InstrumentedModel class that extends Model (the non-instrumented) and adds a better API layer that does get the class for them (as we're already doing today).

[ipolevoy]

well, I wanted to implemnt this years ago, but did not for a reason. It will be hard to explain to peope about two parallel APIs and instrumentation process. Adding this feature will eliminate instrumentation and add new API. I'm not sold this is a good solution. In fact, I cannot say I like it at all. This was just an idea, and I brushed it off. Instead, Dynamic instrumentaiton was more promising.

[ipolevoy]

I can see that you added a lot to https://github.com/ericbn/activejdbc/wiki, but each will have to have a discussion. Not sure I understand the value.

[ericbn]

I was exercising on a fluent non-instrumented interface there. Well, I think it looks good somehow. Still needing improvement, lots of discussion, and being put to the test, of course! ;- )

Something about fluent interfaces that I feel is prone to errors, is having non-terminating methods. For example, update("bonus = ?", 5).where("years > ?", 10); and convert("dob").with(c); both read great to me, but having non-terminating update() and convert() methods and letting the user write just a call to them without getting a compiler or runtime error back, looks dangerous...

I get your point about having two APIs. Indeed, that will make things more complex.

[ericbn]

Having findWhere("name = ?", "John"); and find("dob").where("name = ?", "John");, the latter letting the user choose which columns we wants in the SELECT query, also reads as a good addition to me, and a desirable companion to #238.

[ipolevoy]

Understand your reasoning, but... BUT. ActiveJDBC is a product. And as a product, it needs to evolve as such. This means that it needs to solve some 80% of common problems, and appeal to 95% developers. This is the only way to maintain a project that is small, easy to uderstand and use. Think of this as IPhone. It works, looks beatiful, but only does things one way. Appealing to every request by some guy who has a specialized problem is a sure way to drive this product the Hibernate way - it will do everything, but it will do nothing well. Fluent interfaces have their use, like any other feature (and have their disadvantages, see javalite/activeweb#186). I like them in some places, and would not use them "just because":)

So, @vyemialyanchyk is a great developer, and I actually know him a bit, but so far he is the only person who requested #238..

Making sense?

[punkeel]

Hi,
I think what I'm going to say is linked to this issue, so ....

Is it possible to register a Table<->Class(Model) association at runtime? I see no reason for this to be impossible, but ... Haven't found documentation/API for this.

Thanks

[ipolevoy]

@punkeel, this idea deserves attention. However, this means that someone(something) will need to look at code and generate a property file:
com.akmetools.proj1.Person=PEOPLE

besides, in order ot generate this file, this tool will potentially need a connection the DB. This means we are back to square one: "we need a post-processing tool". But we already have instrumentation:)

So, the biggest problem we need to solve is to somehow know that we are runnign in the context of class Person, and nmot model when we execute:

Person.where()

However Java compiler compiles this code to:

Model.where()

That means that even with a property file, we will now solve this issue. I see no way out of instrumentation.

[punkeel]

Ok thanks, I understand why instrumentation is required then...
Is it possible to play with Generics? I assume that "No it isn't", but why not?

[ericbn]

@punkeel, not sure I understood your question. Do you mean something like calling Person.setTableName("people_table"); to set the table name associated with the model Person, overriding the value set by convention or by a @Table("people") annotation on that class?

[ipolevoy]

guys, no amount of setting the table on a model is going to help.
The biggest problem is when you execute this

Person.where()

you do not know that you are running inside a Person classs. This is because in Java static methods are not inherited. We fix this by using instrumentation - moving the actual byte code for a methods where from Model class to Person class.
If we find a way to know which class we are running without instrumentation, we have solved this problem.

However, I really do not understand why instrumentation is such a big deal for people (http://stackoverflow.com/questions/27364354/activejdbc-instrumentation-unable-to-instrument-the-model-classes-which-are-in/27369214#27369214).

This is a simple step, and should not be a big deal!

[punkeel]

It is a simple step, but it becomes harder when you have to load multiple
jars from plugins :-)

Le lundi 15 décembre 2014, Igor Polevoy notifications@github.com a écrit :

guys, no amount of setting the table on a model is going to help.
The biggest problem is when you execute this

Person.where()

you do not know that you are running inside a Person classs. This is
because in Java static methods are not inherited. We fix this by using
instrumentation - moving the actual byte code for a methods where from
Model class to Person class.
If we find a way to know which class we are running without
instrumentation, we have solved this problem.

However, I really do not understand why instrumentation is such a big deal
for people (
http://stackoverflow.com/questions/27364354/activejdbc-instrumentation-unable-to-instrument-the-model-classes-which-are-in/27369214#27369214).

This is a simple step, and should not be a big deal!

—
Reply to this email directly or view it on GitHub
#328 (comment).

---

Cordialement, PunKeel'.

[ipolevoy]

@punkeel why would you load jars? What plugins?

[punkeel]

@ipolevoy : Bukkit plugin, I have one including your javalite jar and would like others to have access to it. I do instrument the others, but their configurations are like not loaded ... But they are instrumented.
So it could maybe work, at least for my case, with the register(model.class)

[ipolevoy]

@punkeel , sorry I have no idea what that means. Any ActiveJDBC project can be compiled, instrumented and packaged into a jar file. At that point, you can do whatever you want with it, as you would with any other library

[punkeel]

@ipolevoy when ActiveJDBC loads, it does also load the property file generated by instrumentation, isn't it ?
If I'm right, and I think so, it also stores from this file something ... that makes it unable to use instrumented classes "not registered".

The compiled model contains the right methods, the compiled .class using this model contains the right class name, so the static calls are made on the model, not on Model.

I quite don't see how this is possible, then :-) (and I can't do anything but hack the classloader for now)

[ericbn]

@ipolevoy, added two commits in my fork for you to review.

First commit (f26b3a9) makes non-static methods independent of instrumentation. As in non-static context there is this.getClass(), it is used.

Second commit (17565df) eliminates the calls to Model.getDaClass() and ClassGetter, calling only getDaClass() in the subclasses. The previous code structures never allowed this method in the subclasses to be called. One thing that needs to be fixed in this second commit is that it exposes a "non-instrumented" static API (that was private in the previous commit). This is so because static methods in the subclasses need to call these in Model. My suggestion is we keep them private in Model (as in previous commit) and use instrumentation to change them to protected. So we'll need to instrument the Model class. My motivation to think about a good "non-instrumented" API comes from this approach...

[ericbn]

@ipolevoy, the profiling of each commit stage, with its top entries, are transcribed below. These also help understanding another motivation for this refactoring.

Each profiling session ran all our unit tests.

Before first commit

Hot Spots - Method	Self Time [%]	Self Time	Total Time	Invocations
DB.execInsert(s, s, o[])	18.5	1605.1 ms	3354.0 ms	9027
LogFilter.log(l, s)	13.5	1170.1 ms	1190.4 ms	10555
NumericValidator.validate(m)	7.0	607.3 ms	656.7 ms	8032
m$ClassGetter.get()	6.2	535.7 ms	535.7 ms	17614
StatisticsQueue.enqueue(qee)	4.0	347.8 ms	352.2 ms	9552
Inflector.gsub(s, s, s)	3.2	273.5 ms	273.5 ms	63907
DB.open(s, s, s, s)	3.1	272.5 ms	382.0 ms	281
m.doInsert()	2.1	184.7 ms	4143.9 ms	9027
LogFilter.logQuery(l, s, o[], l)	1.8	153.9 ms	1746.4 ms	9491
Util.join(sb, o[], s)	1.5	132.0 ms	133.9 ms	9425

After first commit

Number of calls to ClassGetter dropped from 17614 to 8515, time dropped from 535.7 ms to 200.1 ms.

Hot Spots - Method	Self Time [%]	Self Time	Total Time	Invocations
DB.execInsert(s, s, o[])	18.1	1272.1 ms	2897.4 ms	9027
LogFilter.log(l, s)	15.6	1095.7 ms	1116.0 ms	10555
NumericValidator.validate(m)	6.2	437.7 ms	481.5 ms	8032
StatisticsQueue.enqueue(qee)	4.5	317.9 ms	322.1 ms	9552
Inflector.gsub(s, s, s)	4.0	278.7 ms	278.7 ms	63907
DB.open(s, s, s, s)	3.4	240.2 ms	291.6 ms	281
m$ClassGetter.get()	2.8	200.1 ms	200.1 ms	8515
m.doInsert()	2.5	176.0 ms	3599.5 ms	9027
Util.join(sb, o[], s)	1.8	129.5 ms	131.3 ms	9425

After second commit

No ClassGetter anymore!

Hot Spots - Method	Self Time [%]	Self Time	Total Time	Invocations
DB.execInsert(s, s, o[])	20.2	1531.0 ms	3405.7 ms	9027
LogFilter.log(l, s)	14.4	1088.3 ms	1108.7 ms	10555
NumericValidator.validate(m)	8.0	603.9 ms	663.0 ms	8032
StatisticsQueue.enqueue(qee)	5.4	410.6 ms	414.2 ms	9552
Inflector.gsub(s, s, s)	3.6	273.3 ms	273.3 ms	63907
DB.open(s, s, s, s)	3.2	241.9 ms	293.3 ms	281
m.doInsert()	2.2	162.9 ms	4140.3 ms	9027
LogFilter.logQuery(l, s, o[], l)	2.0	148.7 ms	1774.1 ms	9491
Util.join(sb, o[], s)	1.8	134.3 ms	137.1 ms	9425
m.create(c, o[])	1.4	106.8 ms	292.7 ms	8194

The calls to getDaClass() also changed between commits. See below.

Before first commit

Hot Spots - Method	Self Time [%]	Self Time	Total Time	Invocations
m.getDaClass()	0.4	34.5 ms	613.1 ms	17614

After first commit

Hot Spots - Method	Self Time [%]	Self Time	Total Time	Invocations
m.getDaClass()	0.2	15.5 ms	240.1 ms	8515

After second commit

Hot Spots - Method	Self Time [%]	Self Time	Total Time	Invocations
Person.getDaClass()	0.00015	0.011 ms	0.011 ms	3
Course.getDaClass()	0.00009	0.007 ms	0.007 ms	1
Ingredient.getDaClass()	0.00005	0.004 ms	0.004 ms	1
Recipe.getDaClass()	0.00005	0.004 ms	0.004 ms	1
Student.getDaClass()	0.00005	0.004 ms	0.004 ms	1

Looks like the JVM could optimize the calls in this scenario. Maybe the profiler missed some calls too... (too insignificant?!)

[ipolevoy]

@ericbn, this is super interesting. AJ was 2 times faster than Hibernate before. but has a bigger potential. I will review this close to end of this week

tx

[ipolevoy]

@ericbn , not sure I'm following. Above you are referring to commits on your fork, but they are in master. Am I missing anything?

[ipolevoy]

@ericbn , I found two more commits on your fork. Look OK to me. I'm slightly confused by Git. I assume that you still have some code on your fork that has not been merged with master? I think it is OK to do so

[ipolevoy]

yeah, I think a more comprehensive profiling is needed. Back in 2010, I was dealing with performance optimization, and created some tests in ActiveJDBC and Hibernate to do exactly the same: store and retrieve a large number of data to/from MySQL. On the first run, ActiveJDBC turned out to be a dog, about 10 times slower than Hibernate. Code here: https://dl.dropboxusercontent.com/u/43668168/DBExamples.zip
So, I found some issues in design where AJ was spending too much time creating new models and optimized it to run about 2 times faster than Hibernate. After that, any optimization became a moot point. Since we made so many changes recently, it makes sense to repeat these tests (have not done so since 2010:)) and see if ActiveJDBC is still fast. I would not be obsessing with method calls. I'm pretty sure time will be spent on creating temp objects, IO calls to DB, etc.

[ericbn]

Hi @ipolevoy. What happened with the commits was I pushed these by mistake to the javalite/master and immediately reverted the changes then. What git does in these cases is it keeps the commits but removes them from the branch. I'll merge them again. ;- )

I'll also investigate the profiling and comparison with Hibernate. We can automate this in our CI env. What do you think?

Are you aware that these new changes we are merging expose a new API (with protected visibility) that is a duplicate of the original API only with an added first parameter that is the model class? Do you think we should (1) keep it as it is, (2) hide it with instrumentation of the Model class, or (3) improve it?

[ipolevoy]

@ericbn , I think this deserves a more in-depth discussion on Skype. Let me know when you want to talk .

[ericbn]

Sure, @ipolevoy! Just came back from my vacations...

[ipolevoy]

@ericbn - no pressure :)

[ipolevoy]

this was completed some time ago