Bug 20296

Summary: OpcodeStats doesn't build on platforms which don't have mergesort()
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Minor CC: ggaren, mjs, oliver
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
proposed patch
eric: review-
new patch based on Comment #2
none
This is my new proposed patch.
darin: review-
proposed patch darin: review+

Description Csaba Osztrogonác 2008-08-06 04:35:24 PDT
OpcodeStats don't build on linux, beacuse on this platform there isn't mergesort() function. qsort() should be used instead of mergesort().
Comment 1 Csaba Osztrogonác 2008-08-06 04:39:54 PDT
Created attachment 22676 [details]
proposed patch
Comment 2 Eric Seidel (no email) 2008-08-06 05:20:32 PDT
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)
Comment 3 Csaba Osztrogonác 2008-08-06 05:37:48 PDT
(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?
Comment 4 Csaba Osztrogonác 2008-08-07 04:32:54 PDT
Created attachment 22694 [details]
new patch based on Comment #2

I refactored using mergesort based on your advices. What about this preliminary patch?
Comment 5 Csaba Osztrogonác 2008-08-22 04:33:40 PDT
Created attachment 22933 [details]
This is my new proposed patch.
Comment 6 Darin Adler 2008-08-22 08:41:42 PDT
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.
Comment 7 Darin Adler 2008-08-22 08:42:01 PDT
My apologies -- I'm not trying to make this more complicated than it is.
Comment 8 Csaba Osztrogonác 2008-08-25 00:42:51 PDT
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.
Comment 9 Csaba Osztrogonác 2008-08-25 00:44:15 PDT
Created attachment 22974 [details]
proposed patch
Comment 10 Darin Adler 2008-08-25 08:58:58 PDT
Comment on attachment 22974 [details]
proposed patch

r=me
Comment 11 Mark Rowe (bdash) 2008-09-02 23:29:02 PDT
Landed in r36048.