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

Already on GitHub? Sign in to your account

Could not exclude result/resultset and performance issues #84

Closed
quintonm opened this Issue Oct 12, 2013 · 6 comments

Comments

Projects
None yet
3 participants
Owner

quintonm commented Oct 12, 2013

Migrated from sourceforge: https://sourceforge.net/p/p6spy/bugs/9/

I downloaded the newest version today. While I put it
into my system, I found that the "result" could not be
excluded. And I noticed that in the manual, it is said
"result" while in log it's "resultset". So I add
"resultset" to the excluded list. Nothing happens. So I
refer to the source code. Em, I catch it.
In P6ResultSet.java ( I does not use the real code
here, because this box has no source code. :( )
public boolean next() throws SQLException {
// create the string buffer;
// log directly
// return proxyObject.next();
}
Although the sub-class "P6LogResultSet" re-write the
next() method, but it's a wrong way as the following:
public boolean next() throws SQLException {
try {
return super.next();
}
catch ( Exception ex ) {
// log with contriol
}
}
So it's clear where cause the trouble. And in
P6ResultSet.next(), there is a performance issue. Why
not check if log needed first here? Think about a
resultset with over 10K rows and each row has more than
100 columns.
After all, I am glad to see all the SQL's our system
sending to the DBMS. A good job. We can make it better.

Owner

quintonm commented Oct 12, 2013

sourceforge comment:

I also noticed this. It's swamping all my logging, I can't exclude the resultset output.

sourceforge comment:

I should add, to be more specific you can't filter the result set output. It's calling directly into P6LogQuery.log() as opposed to something like logElapsed which will call isLoggable.

jsimao71 commented Nov 2, 2013

Hi, Guys! Thanks alot for p6spy!
This is actually a major issue..can you please fix it with urgency/high-priority!??
Should not be that hard for people already very familiar with the code base.
Thanks in advance,
Jorge.

Owner

typekpb commented Nov 2, 2013

@jsimao71 thanks for feedback. As far as I see, there are 2 things reported here:

  • resultset exclusion - that is already fixed, see #86 and
  • performance problem - that might need deeper check, as we're using proxies and things behave a bit differently

so if you're affected by the 1.st one, it's already covered

Owner

typekpb commented Nov 20, 2013

to check performance, issue has been raised #123. Let's see what it shows.

@quintonm quintonm added the Blocked label Mar 17, 2014

Owner

typekpb commented Mar 18, 2014

The initial version of performance tests setup, see: https://github.com/p6spy/p6spy-perf

In the scope of initial tests, following has been done:

results on my test machine (4x i5-2430M CPU @ 2.40GHz, 8GB RAM) follow:

jdbc request average [ms]
"no-p6spy select" 326
"p6spy select" 1286

well, that is very poor. Sounds like we need to do some profiling here.

@typekpb typekpb removed the Blocked label Mar 19, 2014

@typekpb typekpb self-assigned this Mar 21, 2014

typekpb added a commit that referenced this issue Mar 23, 2014

Owner

typekpb commented Mar 23, 2014

some improvements has been done, see the referred commits for details.

To document impact, the same perf tests have been used (but repetitions count has been increased from 5 to 10).

pre updates results (using p6spy:p6spy:2.0.1):

jdbc request average [ms]
"no-p6spy select" 259
"p6spy select" 1232

post updates results (using p6spy:p6spy:2.0.2-SNAPSHOT)

jdbc request average [ms]
"no-p6spy select" 434
"p6spy select" 776

well, it seems that with such a low repetition count, results might not be super accurate, still the performance improvement is obvious.

@typekpb typekpb removed their assignment Jun 17, 2014

@quintonm quintonm closed this May 9, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment