Possible mistakes in com.sun.javafx.geom.AreaOp
John Hendrikx
john.hendrikx at gmail.com
Sun Dec 1 21:40:28 UTC 2024
Yeah, that AWT code is changed almost exactly the same way. Don't know
why they kept Vector, could be AWT specific reason that doesn't apply to
FX. Uncontested synchronized blocks should however be practically free,
so I don't think it's worth doing more than copying their changes.
--John
On 30/11/2024 18:51, Nir Lisker wrote:
> You're right, these geometry classes were copied from AWT. Here is the
> offending
> AreOp: https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/sun/awt/geom/AreaOp.java.
>
>
> They generified their code at some point like John did. They also just
> create a new vector in the 'numedges' check. I wonder why they kept
> Vector though.
>
> On Sat, Nov 30, 2024 at 7:43 PM Laurent Bourgès
> <bourges.laurent at gmail.com> wrote:
>
> Probably derived from java.awt.Area:
> https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/java/awt/geom/Area.java
>
>
> Le sam. 30 nov. 2024, 17:59, Nir Lisker <nlisker at gmail.com> a écrit :
>
> I agree with your type analysis. However, I'm hesitant to
> change the logic in regarding 'numedges' without someone who
> is familiar with the domain taking a look; these sorts of
> computations often have non-obvious sides to them.
>
> I also think that the class was copied. The raw types usage
> suggests a 1.4 era. Vector itself suggests even earlier times,
> perhaps AWT. If the code doesn't require thread
> synchronization, and I don't think it does, using List could
> even speed it up a bit.
>
> Do you have an idea about the needless 'toArray' calls? The
> return is not captured and it doesn't have side effects. Maybe
> to check the types?
>
> On Sat, Nov 30, 2024 at 10:25 AM John Hendrikx
> <john.hendrikx at gmail.com> wrote:
>
> Hi Nir,
>
> I encountered that class before while doing raw warning
> clean-ups in graphics (which were never integrated).
>
> The problem IMHO is in the assignment in `calculate`:
>
> edges = pruneEdges(edges);
>
> This assignment is both confusing and unnecessary, and
> violates the principle of re-using variables for different
> purposes. Method pruneEdges must return CURVES (the name
> even implies it -- remove edges), and the debug code in
> this block even assumes it does (it assigned `numcurves`
> to `edges.size()` then assumes that it contains curves
> when converting to the array format):
>
> Rewrite the code as follows and it will be much clearer:
>
> publicVector<Curve>calculate(Vector<Curve> left,
> Vector<Curve> right) {
>
> Vector<Edge> edges = newVector<>();
>
> addEdges(edges, left, AreaOp.CTAG_LEFT);
>
> addEdges(edges, right, AreaOp.CTAG_RIGHT);
>
> Vector<Curve> curves = pruneEdges(edges);
>
> if(false) {
>
> System.out.println("result: ");
>
> intnumcurves = curves.size();
>
> Curve[] curvelist = (Curve[])
> edges.toArray(newCurve[numcurves]);
>
> for(inti = 0; i < numcurves; i++) {
>
> System.out.println("curvelist["+i+"] = "+curvelist[i]);
>
> }
>
> }
>
> returncurves;
>
> }
>
> Then the initial code in `pruneEdges` is just plain wrong,
> and seems to be an attempt at optimization done
> incorrectly by returning an existing vector to save having
> to allocate a new one:
>
> privateVector<Curve>pruneEdges(Vector<Edge> edges) {
>
> intnumedges = edges.size();
>
> if(numedges < 2) {
>
> returnedges;
>
> }
>
> Method pruneEdges however is supposed to return a minimum
> set of curves that enclose an area. A single edge can't
> contribute any area. I'm pretty sure that if you change
> the check to `numedges < 1` that if the remaining code
> runs normally it would also come to that conclusion (it
> would return an empty vector of curves). So this
> "optimization" here is doing the wrong thing by
> potentially returning a single edge instead of always
> returning an empty curve vector.
>
> Now, I'm pretty sure we'll never see this case in
> practice, due to other checks being done (specifically the
> `getOrder() > 0` check when adding edges) and the likely
> fact that there is always going to be a minimum of 2
> curves being passed to `calculate. Adding an assert or
> just modifying the code, and then running all tests may
> help verify this. I believe however that the code needs
> to be this:
>
> privateVector<Curve>pruneEdges(Vector<Edge> edges) {
>
> intnumedges = edges.size();
>
> if(numedges < 2) {
>
> returnnumedges == 0 ? (Vector<Curve>)(Vector<?>)edges :
> new Vector(); // as a single edge can't encompass any
> area, a single edge also results in no curves being returned
>
> }
>
> The reason why we do need to return a mutable Vector here
> is because the classes using the return value of calculate
> assume that it is writable and doesn't need copying. The
> ugly double cast of edges is relatively contained -- it is
> only casted when empty (to avoid creating another vector
> instance), and it was created by this class so its under
> control. Always returning `new Vector()` would also work,
> but it may perform a tiny bit slower in some cases
> (probably not measurable IMHO).
>
> I suspect the entire class is copied from somewhere, as at
> the time JavaFX was written using Vector while at the same
> time trying to do these kinds of optimizations is odd to
> say the least.
>
> --John
>
>
> On 30/11/2024 00:12, Nir Lisker wrote:
>> I came across a potential mistake in the
>> class com.sun.javafx.geom.AreaOp. It uses raw Vector
>> types and while trying to add generic parameters there
>> for type safety, I got some conflicts.
>>
>> In the method AreaOp::calculate, the arguments should be
>> Vector<Curve> and the return type should also be
>> Vector<Curve>, but it returns a Vector called 'edges'.
>> 'edges' is passed to the 'addEdges' method that should
>> take Vector<Edge> and Vector<Curve>. This means that
>> 'edges' is a Vector<Edge> and not Vector<Curve>. Already
>> a conflict.
>> Then it is passed to 'pruneEdges', which, if it takes and
>> returns Vector<Edge>, runs into a conflict with the
>> return type: 'Vector ret' has Curve added to it. If I try
>> to return Vector<Curve> instead, then in the 'numedges <
>> 2' check the input Vector can't be returned unless it's
>> also Vector<Curve>, which again causes a conflict in
>> 'calculate'.
>>
>> There are also 'toArray' calls that seem to not do
>> anything, like in 'resolveLinks' (line 454) and in
>> 'finalizeSubCurves' (429).
>>
>> Can anyone who knows this part of the code take a look?
>>
>> - Nir
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20241201/90139774/attachment-0001.htm>
More information about the openjfx-dev
mailing list