Simon evaluation

33 views
Skip to first unread message

Erik van Oosten

unread,
Dec 24, 2008, 6:34:14 PM12/24/08
to javasimon
Hello,

I wrote an article with the results of my Simon evaluation:
http://day-to-day-stuff.blogspot.com/2008/12/evaluating-simon.html

I hope you like it. Comments are welcome.

Enjoy the end of the year!
Erik.

--
Erik van Oosten
http://day-to-day-stuff.blogspot.com/

Virgo47

unread,
Dec 24, 2008, 8:15:48 PM12/24/08
to javasimon
Hi Erik

First I greatly appreciate the article because it let me again to go
through a few things I did in the Simon core. There is one point I can
fully agree... sample and map. Map seems so ... easy to process,
however normal Java Bean is probably as good - or maybe even some
"Sample" class that can contain some metadata. I can also create two
sample methods - sample() and sampleAndReset() to fix other little
issues you've found. It would smash magic values problem too.

For other-than-sample usages I don't like having other object that
would contain measurement data instead of convenient getters available
directly on the Simon.

Next thing is thread local in the Stopwatch and more ways how the
Stopwatch can be used in multithreaded environment. Jamon does the
same you suggest (sort of) - it creates a facade for monitor every
time you want it and this frontend holds reference to "monitor data".
You actually never work with the object that holds the data - only
with the object that can measure one split in parallel and it holds
the object that holds the data. ;-) Simon is different, Stopwatch is
heavier but it's only one object for one Stopwatch you work with -
ThreadLocal is the price. I don't like the hack you did - I guess it
was only a concept proof - because you can do:

Stopwatch sw = SimonManager.getStopwatch("blabla");
sw.start();
...
sw.stop();

start returns object for convenience and - although I did not think
about it that time - I guess it must be this with current interface.
Other option is that you can obtain Stopwatch either to do something
with it except to measure, and than you can start it. Start would
return your proposed object - and in that case it need not to be
Stopwatch anymore. I may rather see Split object not being confused
with Stopwatch when it can't do what you expect Stopwatch to do
(stopwatch related getters, common Simon functionality).

Now I think Stopwatch is quite consistent which doesn't mean I don't
see your points. Map & memory leak is something I never thought about.
"Keyed" start/stop is quite nice way how to tie one measurement to
some object you can't control (not your implementation), to track its
lifetime or whatever. Still you're right about the missing-stop issue
and I'll have to think about this more. Small object related to one
Split measurement would fix this as it should be GCed when someone
forget it. On the other hand client has to keep his own map object-
>split especially when it's not possible to put reference to Split
into the tracked object (not your implementation for instance). This
topic I consider open - but no changes for now (I don't know if now is
weak, month or year - honestly). (Can't this be fixed with some map
with weaker-references too?)

Last thing - I'm quite happy that there is no recommended change in
EnabledManager in the end. I was considering synchronization only on
the map (or using proposed ConcurrentHashMap for that matter), but I
went for synchronizing the whole method for simple reason. The code
there is quite... hard to admit... ugly. It does complex stuff that is
supported with quite a few unit tests because there were some tricky
bugs there - mostly related with keeping the Simon hierarchy
consistent. With code doing so many things, reading and later adding
to the map... then there is some get for parent... I rather
synchronized it completely. There may be some way how to break the
code into something better and narrow the synchronized part - but it
is over my head now. With synchronized there I feel quite safe - I
can't imagine proper unit tests to cover concurrency in case of add/
remove Simons from the tree.

Thank you for quite a thorough article - review is always important
when we want to continue with the Simon to the future. And we do. :-)
Nice x-mas gift from you.

Nice holidays and end of the year to you too!

Virgo

Erik van Oosten

unread,
Dec 25, 2008, 4:46:50 AM12/25/08
to javasimon
Hi Virgo,

Well, you're certainly at it! Even at Christmas.

I just added a section on iterating the simon tree.

Regards,
Erik.


On 25 dec, 02:15, Virgo47 <virg...@gmail.com> wrote:
> Hi Erik
>
> First I greatly appreciate the article because it let me again to go...

Virgo47

unread,
Dec 25, 2008, 6:47:27 AM12/25/08
to javasimon
Ok, valid point. I didn't realize that wrapping doesn't solve the
concurrent modification issue - my fault. Why tree set? I wanted the
tree to be ordered right away - but it's probably not so important
like to remove this bug. I could alternatively copy the set every time
- but that's unnecessarily expensive. We'll probably go for your
suggested fix soon.

Thanks

Virgo

Erik van Oosten

unread,
Dec 28, 2008, 10:11:55 AM12/28/08
to java...@googlegroups.com
Reply below...

Virgo47 wrote:
> First I greatly appreciate the article because it let me again to go
> through a few things I did in the Simon core. There is one point I can
> fully agree... sample and map. Map seems so ... easy to process,
> however normal Java Bean is probably as good - or maybe even some
> "Sample" class that can contain some metadata.

Sample class sounds perfect.

> I can also create two
> sample methods - sample() and sampleAndReset() to fix other little
> issues you've found. It would smash magic values problem too.
>

Great!


> For other-than-sample usages I don't like having other object that
> would contain measurement data instead of convenient getters available
> directly on the Simon.
>

Why? Is there a particular use case you have in mind?
I think it keeps the interface nicely small and clean.

> Next thing is thread local in the Stopwatch and more ways how the
> Stopwatch can be used in multithreaded environment. Jamon does the
> same you suggest (sort of) - it creates a facade for monitor every
> time you want it and this frontend holds reference to "monitor data".
> You actually never work with the object that holds the data - only
> with the object that can measure one split in parallel and it holds
> the object that holds the data. ;-)

Hmm, that's I would have done as well...


> Simon is different, Stopwatch is
> heavier but it's only one object for one Stopwatch you work with -
>

Interesting. I'll have to see the code of Jamon to understand why it is
so much slower. Can't be just this, can it?


> ThreadLocal is the price. I don't like the hack you did - I guess it
> was only a concept proof

Yes, that was a proof of concept. To really go with this approach the
interface should change in an incompatible way. Unfortunately.

> - because you can do:
>
> Stopwatch sw = SimonManager.getStopwatch("blabla");
> sw.start();
> ...
> sw.stop();
>
> start returns object for convenience and - although I did not think
> about it that time -

You didn't do that for nothing. It was a sign that there is more going on :)


> I guess it must be this with current interface.
> Other option is that you can obtain Stopwatch either to do something
> with it except to measure, and than you can start it. Start would
> return your proposed object - and in that case it need not to be
> Stopwatch anymore. I may rather see Split object not being confused
> with Stopwatch when it can't do what you expect Stopwatch to do
>

Right, Split is a better word then my proposed StopwatchMeasurement.
Indeed, it should not be a Stopwatch. It should only have the stop
method (although, it is the end of the split, so perhaps 'end' is a
better method name).

> (stopwatch related getters, common Simon functionality).
>
> Now I think Stopwatch is quite consistent which doesn't mean I don't
> see your points. Map & memory leak is something I never thought about.
> "Keyed" start/stop is quite nice way how to tie one measurement to
> some object you can't control (not your implementation), to track its

> lifetime or whatever. ..........


> (Can't this be fixed with some map
> with weaker-references too?)
>

You could use weak references. Problem is that these tend to be heavy
for the GC, you should not use these too often.

> ...... The code
> there is quite... hard to admit... ugly. It does complex stuff ......


> There may be some way how to break the
> code into something better and narrow the synchronized part - but it
> is over my head now.

Hmm, that is not a good sign. It will probably come to you when you let
it rest for a while and then track a few use cases :)

> With synchronized there I feel quite safe - I
> can't imagine proper unit tests to cover concurrency in case of add/
> remove Simons from the tree.
>

It is possible, but hard. Clean Code refers to ConTest, a tool from IBM
that helps you test concurrent code. The 'jiggling' I used in the
performance application also helps.

> Thank you for quite a thorough article - review is always important
> when we want to continue with the Simon to the future. And we do. :-)
> Nice x-mas gift from you.
>
>

Thanks for the open mind!

Regards,
Erik.


--
Erik van Oosten
http://www.day-to-day-stuff.blogspot.com/

Virgo47

unread,
Jan 7, 2009, 5:20:25 PM1/7/09
to javasimon
Hi Erik (along with everyone on the mailing list ;-))

I've just committed some changes to the Simon that breaks the
compatibility towards some of your suggestions:
* stopwatch.start() returns Split, you can stop() it... there is no
other way how to measure splits, no hidden maps anymore
* sample/sampleAndReset() return Sample... it's a Java Bean... at this
time very plain - question is if it needs to be anything more
anyway...
* children collection is that copy-on-write list... too bad children
are not sorted anymore ;-)

At this moment all "business" getters on Simons are still there...
doesn't mean they will stay, but I guess they don't matter now. Hope
you like this version better - I found working with Splits quite nice
I have to say. It's now fully up to the client code programmer how he
manages any multithreaded measurement but for most cases there is easy
way how/where to keep Split object which makes it just as simple as in
the original version. Also one way how to measure splits (with an
explicit objects that represent them) forces the programmer to think
about it instead of running into the wrong pair of start/stop methods
available in the previous version.

Please, look at this version (checkout/update, no artifacts to
download) and tell me what you think.

Thank you

Virgo
Reply all
Reply to author
Forward
0 new messages