Re: RFR: 8240692: Cleanup of the javafx property objects

[Kevin Rushforth]

On Mon, 16 Mar 2020 12:47:44 GMT, Jeanette Winzenburg  
wrote:

>>> So this begs the question: Do we still want to do this? Is the cleanup 
>>> worth the extra memory used?
>> 
>> I'm not sure. This raises the opposite question: are there any places where 
>> we can benefit from introducing anonymous
>> classes instead of the current implementation?
>> In my own code, I prefer the readability and conciseness over performance. 
>> However, since JavaFX is a library, we might
>> want to go with the performance route. I don't know how important the memory 
>> consumption is for users in this case or
>> the other similar cases.
>
> Just playing devil's advocate :)
> 
> Technically, there's a functional difference between the inline extension and 
> useage of SimpleXX: in the former,
> getName returns the current name of mapped property, in the latter it returns 
> its name at the time of creating the
> mapped property.   Afaics, there is nothing in the spec of getName that 
> prevents custom implementations with name (and
> bean) being mutable (the SimpleXX have implemented them immutable, but then 
> they are only default implementations). Not
> that I can imagine any use-case for it, but if they exist, the new mappings 
> might break existing code.

Getting back to this, I think we shouldn't make the change from inner classes 
to SimpleXxxxProperty objects proposed by
this PR.

As for whether we should consider changing some internal instance of 
SimpleXxxxProperty (which I think wouldn't run
afoul of the hypothetical problem raised by @kleopatra ), we could file a new 
Enhancement request if you like, but it's
probably not all that critical, unless we were to find a few in the base Node 
or Control classes that could yield big
savings.

-

PR: https://git.openjdk.java.net/jfx/pull/141

Re: RFR: 8240692: Cleanup of the javafx property objects

[Jeanette Winzenburg]

On Sun, 15 Mar 2020 20:48:57 GMT, Nir Lisker  wrote:

>> I ran tests to validate my above assumption, and the results match my 
>> expectations. Using a `SimpleXProperty` takes
>> an extra 8 bytes (one object reference) of storage versus an anonymous inner 
>> class. I used a null bean and a `this$0`
>> reference to get the name. I measured it by checking the memory needed for 
>> 1,000,000 instances of an anonymous subclass
>> of `DoublePropertyBase` objects versus 1,000,000 instances of 
>> `SimpleDoubleProperty`. The former takes 40 bytes per
>> instance while the latter takes 48 bytes. This represents a 20% increase in 
>> memory used.  To see how much this affects
>> the case of the wrapped property objects, I ran two tests on the current 
>> code and the same two tests with your proposed
>> fix. The first test creates an `ObjectProperty` object from a 
>> `DoubleProperty` using
>> `DoubleProperty::asObject`. The second test created a `DoubleProperty` 
>> object from an `ObjectProperty` using
>> `DoubleProperty::doubleProperty`.  I got the same results as for the earlier 
>> tests: 8 additional bytes (one object
>> reference) using `SimpleXxxxProperty` to create the new wrapped property 
>> object. Since these objects are already fairly
>> large due to the bidirectional binding created between the original property 
>> and the wrapping property, the percentage
>> increase is not all that great -- about 3.8% in the case of `Property`.  
>> | Method | bytes/object current |
>> bytes/object with patch | | --- | --- | --- | | DoubleProperty::asObject | 
>> 232 | 240 | | DoubleProperty::doubleProperty
>> | 208 | 216 |  So this begs the question: Do we still want to do this? Is 
>> the cleanup worth the extra memory used?
>
>> So this begs the question: Do we still want to do this? Is the cleanup worth 
>> the extra memory used?
> 
> I'm not sure. This raises the opposite question: are there any places where 
> we can benefit from introducing anonymous
> classes instead of the current implementation?
> In my own code, I prefer the readability and conciseness over performance. 
> However, since JavaFX is a library, we might
> want to go with the performance route. I don't know how important the memory 
> consumption is for users in this case or
> the other similar cases.

Just playing devil's advocate :)

Technically, there's a functional difference between the inline extension and 
useage of SimpleXX: in the former,
getName returns the current name of mapped property, in the latter it returns 
its name at the time of creating the
mapped property.

Afaics, there is nothing in the spec of getName that prevents custom 
implementations with name (and bean) being mutable
(the SimpleXX have implemented them immutable, but then they are only default 
implementations). Not that I can imagine
any use-case for it, but if they exist, the new mappings might break existing 
code.

-

PR: https://git.openjdk.java.net/jfx/pull/141

Re: RFR: 8240692: Cleanup of the javafx property objects

[Nir Lisker]

On Fri, 13 Mar 2020 15:02:48 GMT, Kevin Rushforth  wrote:

> So this begs the question: Do we still want to do this? Is the cleanup worth 
> the extra memory used?

I'm not sure. This raises the opposite question: are there any places where we 
can benefit from introducing anonymous
classes instead of the current implementation?

In my own code, I prefer the readability and conciseness over performance. 
However, since JavaFX is a library, we might
want to go with the performance route. I don't know how important the memory 
consumption is for users in this case or
the other similar cases.

-

PR: https://git.openjdk.java.net/jfx/pull/141

Re: RFR: 8240692: Cleanup of the javafx property objects

[Kevin Rushforth]

On Sat, 7 Mar 2020 14:11:59 GMT, Kevin Rushforth  wrote:

>> From what I see, they save the same data.
>
> The anonymous subclasses don't have an instance field for the `bean` or the 
> `name`, so the tradeoff is one of static
> footprint (an extra anonymous inner class) for dynamic footprint (slightly 
> larger instances).
> Note, however, that this savings is reduced by the fact that the inner 
> classes have a `this$0` pointer to the enclosing
> class. I think this means that we are really only saving one reference 
> variable per instance of the anonymous inner
> class versus an instance of SimpleXxxxProperty. That's why I think that in 
> the specific case of this proposed fix, it
> should be fine. I just want to take a closer look.

I ran tests to validate my above assumption, and the results match my 
expectations. Using a `SimpleXProperty` takes
an extra 8 bytes (one object reference) of storage versus an anonymous inner 
class. I used a null bean and a `this$0`
reference to get the name. I measured it by checking the memory needed for 
1,000,000 instances of an anonymous subclass
of `DoublePropertyBase` objects versus 1,000,000 instances of 
`SimpleDoubleProperty`. The former takes 40 bytes per
instance while the latter takes 48 bytes. This represents a 20% increase in 
memory used.

To see how much this affects the case of the wrapped property objects, I ran 
two tests on the current code and the same
two tests with your proposed fix. The first test creates an 
`ObjectProperty` object from a `DoubleProperty`
using `DoubleProperty::asObject`. The second test created a `DoubleProperty` 
object from an `ObjectProperty`
using `DoubleProperty::doubleProperty`.

I got the same results as for the earlier tests: 8 additional bytes (one object 
reference) using `SimpleXxxxProperty`
to create the new wrapped property object. Since these objects are already 
fairly large due to the bidirectional
binding created between the original property and the wrapping property, the 
percentage increase is not all that
great -- about 3.8% in the case of `Property`.

| Method | bytes/object current | bytes/object with patch |
| --- | --- | --- |
| DoubleProperty::asObject | 232 | 240 |
| DoubleProperty::doubleProperty | 208 | 216 |

So this begs the question: Do we still want to do this? Is the cleanup worth 
the extra memory used?

-

PR: https://git.openjdk.java.net/jfx/pull/141

Re: RFR: 8240692: Cleanup of the javafx property objects

[Kevin Rushforth]

On Sat, 7 Mar 2020 00:49:28 GMT, Nir Lisker  wrote:

>> the subclass saves the owner field who is a static null, not?
>
> From what I see, they save the same data.

The anonymous subclasses don't have an instance field for the `bean` or the 
`name`, so the tradeoff is one of static footprint (an extra anonymous inner 
class) for dynamic footprint (slightly larger instances).

Note, however, that this savings is reduced by the fact that the inner classes 
have a `this$0` pointer to the enclosing class. I think this means that we are 
really only saving one reference variable per instance of the anonymous inner 
class versus an instance of SimpleXxxxProperty. That's why I think that in the 
specific case of this proposed fix, it should be fine. I just want to take a 
closer look.

-

PR: https://git.openjdk.java.net/jfx/pull/141

Re: RFR: 8240692: Cleanup of the javafx property objects

[Nir Lisker]

On Sat, 7 Mar 2020 00:30:21 GMT, Tom Schindl 
 wrote:

>> That doesn't seem right. The additional fields are captured in the
>> anonymous class anyway (same as in lambdas).
>> 
>> On Sat, Mar 7, 2020 at 1:53 AM Tom Schindl  wrote:
>> 
>>> I can somehow remember asking Richard Bair why JavaFX internally does not
>>> use Simple* but creates the anonymous subclasses and he said it's memory
>>> reason - Simple* uses more memory because of the additional fields
>>>
>>> —
>>> You are receiving this because you were assigned.
>>> Reply to this email directly, view it on GitHub
>>> ,
>>> or unsubscribe
>>> 
>>> .
>>>
>
> the subclass saves the owner field who is a static null, not?

>From what I see, they save the same data.

-

PR: https://git.openjdk.java.net/jfx/pull/141

Re: RFR: 8240692: Cleanup of the javafx property objects

[Tom Schindl]

On Fri, 6 Mar 2020 23:35:56 GMT, Nir Lisker  wrote:

>> I note that this also changes the wrapper property objects from anonymous 
>> subclasses of XPropertyBase to SimpleXProperty. This is more than 
>> just a readability cleanup. It's probably fine for this case, but that's why 
>> I want a second reviewer.
>
>> I note that this also changes the wrapper property objects from anonymous 
>> subclasses of XPropertyBase to SimpleXProperty. This is more than 
>> just a readability cleanup. It's probably fine for this case, but that's why 
>> I want a second reviewer.
> 
> Isn't SimpleXProperty exactly made for XPropertyBase with the 
> built-in overrides for the bean and the name? When is this substitution not 
> fine?

I can somehow remember asking Richard Bair why JavaFX internally does not use 
Simple* but creates the anonymous subclasses and he said it's memory reason - 
Simple* uses more memory because of the additional fields

-

PR: https://git.openjdk.java.net/jfx/pull/141

Re: RFR: 8240692: Cleanup of the javafx property objects

[Tom Schindl]

On Sat, 7 Mar 2020 00:22:59 GMT, Nir Lisker  wrote:

>> I can somehow remember asking Richard Bair why JavaFX internally does not 
>> use Simple* but creates the anonymous subclasses and he said it's memory 
>> reason - Simple* uses more memory because of the additional fields
>
> That doesn't seem right. The additional fields are captured in the
> anonymous class anyway (same as in lambdas).
> 
> On Sat, Mar 7, 2020 at 1:53 AM Tom Schindl  wrote:
> 
>> I can somehow remember asking Richard Bair why JavaFX internally does not
>> use Simple* but creates the anonymous subclasses and he said it's memory
>> reason - Simple* uses more memory because of the additional fields
>>
>> —
>> You are receiving this because you were assigned.
>> Reply to this email directly, view it on GitHub
>> ,
>> or unsubscribe
>> 
>> .
>>

the subclass saves the owner field who is a static null, not?

-

PR: https://git.openjdk.java.net/jfx/pull/141

Re: RFR: 8240692: Cleanup of the javafx property objects

[Nir Lisker]

On Fri, 6 Mar 2020 23:35:56 GMT, Nir Lisker  wrote:

>> I note that this also changes the wrapper property objects from anonymous 
>> subclasses of XPropertyBase to SimpleXProperty. This is more than 
>> just a readability cleanup. It's probably fine for this case, but that's why 
>> I want a second reviewer.
>
>> I note that this also changes the wrapper property objects from anonymous 
>> subclasses of XPropertyBase to SimpleXProperty. This is more than 
>> just a readability cleanup. It's probably fine for this case, but that's why 
>> I want a second reviewer.
> 
> Isn't SimpleXProperty exactly made for XPropertyBase with the 
> built-in overrides for the bean and the name? When is this substitution not 
> fine?

That doesn't seem right. The additional fields are captured in the
anonymous class anyway (same as in lambdas).

On Sat, Mar 7, 2020 at 1:53 AM Tom Schindl  wrote:

> I can somehow remember asking Richard Bair why JavaFX internally does not
> use Simple* but creates the anonymous subclasses and he said it's memory
> reason - Simple* uses more memory because of the additional fields
>
> —
> You are receiving this because you were assigned.
> Reply to this email directly, view it on GitHub
> ,
> or unsubscribe
> 
> .
>

-

PR: https://git.openjdk.java.net/jfx/pull/141

Re: RFR: 8240692: Cleanup of the javafx property objects

[Nir Lisker]

On Fri, 6 Mar 2020 23:12:57 GMT, Kevin Rushforth  wrote:

>> A simple readability cleanup for the files that were changed in #113.
>> 
>> Note that the boolean property has an `instanceof` check that doesn't exist 
>> in the other properties.
>
> I note that this also changes the wrapper property objects from anonymous 
> subclasses of XPropertyBase to SimpleXProperty. This is more than 
> just a readability cleanup. It's probably fine for this case, but that's why 
> I want a second reviewer.

> I note that this also changes the wrapper property objects from anonymous 
> subclasses of XPropertyBase to SimpleXProperty. This is more than 
> just a readability cleanup. It's probably fine for this case, but that's why 
> I want a second reviewer.

Isn't SimpleXProperty exactly made for XPropertyBase with the built-in 
overrides for the bean and the name? When is this substitution not fine?

-

PR: https://git.openjdk.java.net/jfx/pull/141

Re: RFR: 8240692: Cleanup of the javafx property objects

[Kevin Rushforth]

On Fri, 6 Mar 2020 22:43:50 GMT, Nir Lisker  wrote:

> A simple readability cleanup for the files that were changed in #113.
> 
> Note that the boolean property has an `instanceof` check that doesn't exist 
> in the other properties.

I note that this also changes the wrapper property objects from anonymous 
subclasses of XPropertyBase to SimpleXProperty. This is more than just 
a readability cleanup. It's probably fine for this case, but that's why I want 
a second reviewer.

-

PR: https://git.openjdk.java.net/jfx/pull/141