Bug 20296 – OpcodeStats doesn't build on platforms which don't have mergesort()

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Csaba Osztrogonác]

OpcodeStats don't build on linux, beacuse on this platform there isn't mergesort() function. qsort() should be used instead of mergesort().

[Csaba Osztrogonác]

Created attachment 22676 [details]
proposed patch

[Eric Seidel (no email)]

Comment on attachment 22676 [details]
proposed patch

Nah.  Better would be to either #define mergesort qsort

or to build some sort of abstraction for sorting "platformMergeSort()" and only have the #ifdef in one place.

I'm surprised this hasn't been done already somewhere else?  You might try searching the sources for HAVE(MERGE_SORT)

[Csaba Osztrogonác]

(In reply to comment #2)

> I'm surprised this hasn't been done already somewhere else?  You might try
> searching the sources for HAVE(MERGE_SORT)
I searched for HAVE(MERGESORT), and found it kjs/JSArray.cpp and VM/SamplingTools.cpp. HAVE_MERGESORT macro defined in wtf/Platform.h. I think this is a commonly used macro.

Your advice about platformMergeSort() can be a nice solution. What do you think, if I would refactor my patch and extend it the above mentioned files?

[Csaba Osztrogonác]

Created attachment 22694 [details]
new patch based on Comment #2

I refactored using mergesort based on your advices. What about this preliminary patch?

[Csaba Osztrogonác]

Created attachment 22933 [details]
This is my new proposed patch.

[Darin Adler]

Comment on attachment 22933 [details]
This is my new proposed patch.

This should not be defined in Platform.h -- that goes beyond the scope of what Platform.h should contain; just configuration macros. It belongs in a header like MathExtras.h -- perhaps StdLibExtras.h.

This should be an inline function, not a macro. If it was going to be a macro it should be a macro with arguments, not an unqualified macro.

The name platformSort doesn't make sense to me. If we need mergesort specifically, then it needs to be mergesort. If it can be either qsort or mergesort, then we can come up with a name that describes the reason we want mergesort if we have it -- what is that reason? Why is mergesort better than qsort for these purposes? I don't think "platform" enters into it, despite Eric's earlier suggestion.

For example, in JSArray.cpp it's pretty clear what we want is a stable sort. In those cases, using qsort is actually incorrect, so I don't think we should try to make it more elegant -- the lack of a stable sort there is a problem needs to be fixed.

Is stable sorting the issue in Opcode.cpp too? If we don't have any special requirements, then it we can just change Opcode.cpp to use qsort instead of doing this whole thing.

[Darin Adler]

My apologies -- I'm not trying to make this more complicated than it is.

[Csaba Osztrogonác]

You're right, in JSArray.cpp we need stable sort.

I think we don't need stable sort in Opcode.cpp. OpcodeStats is only generates statistics for developers, it doesn't modify the funcionality of jsc release version.

[Csaba Osztrogonác]

Created attachment 22974 [details]
proposed patch

[Darin Adler]

Comment on attachment 22974 [details]
proposed patch

r=me

[Mark Rowe (bdash)]

Landed in r36048.