On Mon, 14 Aug 2023 13:17:14 GMT, Johan Vos wrote:
>> John Hendrikx has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 16 commits:
>>
>> - Merge branch 'master' of https://git.openjdk.org/jfx into
>>feature/immutable-pseudoclass
On Wed, 16 Aug 2023 14:02:01 GMT, John Hendrikx wrote:
>> Then the map is just an optimization? If the set points to itself then a
>> `Set` as a cache should be enough.
>
> No, a `Set` wouldn't work here, for this reason:
>
> if (CACHE.contains(mutableSet)) {
> return ... //
More than happy to provide feedback regarding performance improvements in our
applications due to this change.
Dirk
> Am 05.09.2023 um 21:25 schrieb Johan Vos :
>
> On Fri, 9 Jun 2023 12:45:02 GMT, John Hendrikx wrote:
>
>>> This fix introduces immutable sets of `PseudoClass` almost everywher
On Tue, 5 Sep 2023 18:45:50 GMT, Andy Goryachev wrote:
> > I think it would best to remove BitSet
>
> does it mean there'll be a followup PR?
Filing a follow-up cleanup bug seems reasonable, but not urgent, especially now
that BitSet is immutable . The problems pointed out are (largely, if not
On Fri, 9 Jun 2023 12:45:02 GMT, John Hendrikx wrote:
>> This fix introduces immutable sets of `PseudoClass` almost everywhere, as
>> they are rarely modified. These are re-used by caching them in a new class
>> `ImmutablePseudoClassSetsCache`.
>>
>> In order to make this work, `BitSet` had t
On Tue, 5 Sep 2023 13:07:55 GMT, John Hendrikx wrote:
> I think it would best to remove `BitSet`
does it mean there'll be a followup PR? or should it be done as a part of this
one?
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1076#discussion_r1316269856
On Mon, 4 Sep 2023 09:40:14 GMT, Johan Vos wrote:
>> John Hendrikx has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 16 commits:
>>
>> - Merge branch 'master' of https://git.openjdk.org/jfx into
>>feature/immutable-pseudoclasss
On Tue, 5 Sep 2023 13:09:46 GMT, John Hendrikx wrote:
>> modules/javafx.graphics/src/main/java/com/sun/javafx/css/BitSet.java line
>> 549:
>>
>>> 547:
>>> 548: for (int i = 0; i < max; i++) {
>>> 549: long m0 = i >= a ? 0 : this.bits[i];
>>
>> Are we guaranteed that it is
On Wed, 16 Aug 2023 14:16:23 GMT, John Hendrikx wrote:
>> Thanks for that test, actually it could be used as part of a test in
>> PseudoClassTest, to verify that the old implementation failed and the new
>> one worked?
>>
>> And this brings up another issue: the constructor `PseudoClassState(L
On Fri, 9 Jun 2023 12:45:02 GMT, John Hendrikx wrote:
>> This fix introduces immutable sets of `PseudoClass` almost everywhere, as
>> they are rarely modified. These are re-used by caching them in a new class
>> `ImmutablePseudoClassSetsCache`.
>>
>> In order to make this work, `BitSet` had t
On Fri, 9 Jun 2023 12:45:02 GMT, John Hendrikx wrote:
>> This fix introduces immutable sets of `PseudoClass` almost everywhere, as
>> they are rarely modified. These are re-used by caching them in a new class
>> `ImmutablePseudoClassSetsCache`.
>>
>> In order to make this work, `BitSet` had t
On Wed, 16 Aug 2023 14:06:16 GMT, John Hendrikx wrote:
>> modules/javafx.graphics/src/main/java/javafx/css/Match.java line 1:
>>
>>> 1: /*
>>
>> The compare method seems wrong. I think it should delegate to
>> `Integer.compare`.
>>
>> `specificity` should also be made `private`.
>
> These wer
On Wed, 16 Aug 2023 13:16:19 GMT, Jose Pereda wrote:
>> You could check this yourself if you want. The `BitSet` class had problems
>> in many places, was largely untested and, to be very honest, should never
>> have passed code review. It violated the `Set` contract almost everywhere,
>> whi
On Wed, 16 Aug 2023 02:02:02 GMT, Nir Lisker wrote:
>> John Hendrikx has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 16 commits:
>>
>> - Merge branch 'master' of https://git.openjdk.org/jfx into
>>feature/immutable-pseudoclas
On Wed, 16 Aug 2023 05:02:36 GMT, Nir Lisker wrote:
>> Almost, but the key is also the copied set. In your version the key is the
>> original object, which may be a modifiable set. If the key is modified, the
>> cache will not function correctly anymore.
>>
>> I had a different version befor
On Tue, 15 Aug 2023 23:05:38 GMT, John Hendrikx wrote:
>> The original ("broken") version has been working fine, and no bugs have been
>> reported so far, and there would be a reason to have a custom implementation
>> instead of the one in `AbstractSet` in the first place.
>>
>> I'm not again
On Tue, 15 Aug 2023 19:33:20 GMT, John Hendrikx wrote:
>> modules/javafx.graphics/src/main/java/com/sun/javafx/css/ImmutablePseudoClassSetsCache.java
>> line 61:
>>
>>> 59: CACHE.put(copy, copy);
>>> 60:
>>> 61: return copy;
>>
>> Isn't this just `return CACHE.computeIfAbsent(
On Fri, 9 Jun 2023 12:45:02 GMT, John Hendrikx wrote:
>> This fix introduces immutable sets of `PseudoClass` almost everywhere, as
>> they are rarely modified. These are re-used by caching them in a new class
>> `ImmutablePseudoClassSetsCache`.
>>
>> In order to make this work, `BitSet` had t
On Tue, 15 Aug 2023 22:21:15 GMT, Jose Pereda wrote:
>> I removed it because my fix requires that `toArray` works correctly. The
>> easiest way to get a correctly working version is to extend `AbstractSet`,
>> which provides a default implementation that works correctly. As I think
>> the de
On Tue, 15 Aug 2023 19:26:20 GMT, John Hendrikx wrote:
>> Okay, thanks for the clarification.
>>
>> So if I get it right, the removal of `toArray` doesn't have to do with the
>> bug, but with the fact that with the immutable set it is not longer required?
>
> I removed it because my fix requir
On Tue, 15 Aug 2023 16:10:46 GMT, Nir Lisker wrote:
>> John Hendrikx has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 16 commits:
>>
>> - Merge branch 'master' of https://git.openjdk.org/jfx into
>>feature/immutable-pseudoclas
On Tue, 15 Aug 2023 14:35:33 GMT, Jose Pereda wrote:
>> It's definitely a bug, but it would only appear if you had bit sets which
>> required more than 1 long to store their data **and** you called `toArray`.
>> It would then throw an `ArrayIndexOutOfBoundsException` in this line:
>>
>> f
On Fri, 9 Jun 2023 12:45:02 GMT, John Hendrikx wrote:
>> This fix introduces immutable sets of `PseudoClass` almost everywhere, as
>> they are rarely modified. These are re-used by caching them in a new class
>> `ImmutablePseudoClassSetsCache`.
>>
>> In order to make this work, `BitSet` had t
On Tue, 15 Aug 2023 14:08:08 GMT, John Hendrikx wrote:
>> modules/javafx.graphics/src/main/java/com/sun/javafx/css/PseudoClassState.java
>> line 65:
>>
>>> 63: /** {@inheritDoc} */
>>> 64: @Override
>>> 65: public T[] toArray(T[] a) {
>>
>> You have mentioned that this was a bug b
On Tue, 15 Aug 2023 11:56:05 GMT, Jose Pereda wrote:
>> John Hendrikx has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 16 commits:
>>
>> - Merge branch 'master' of https://git.openjdk.org/jfx into
>>feature/immutable-pseudocla
On Tue, 15 Aug 2023 11:44:17 GMT, Jose Pereda wrote:
>> John Hendrikx has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 16 commits:
>>
>> - Merge branch 'master' of https://git.openjdk.org/jfx into
>>feature/immutable-pseudocla
On Fri, 9 Jun 2023 12:45:02 GMT, John Hendrikx wrote:
>> This fix introduces immutable sets of `PseudoClass` almost everywhere, as
>> they are rarely modified. These are re-used by caching them in a new class
>> `ImmutablePseudoClassSetsCache`.
>>
>> In order to make this work, `BitSet` had t
On Fri, 9 Jun 2023 12:45:02 GMT, John Hendrikx wrote:
>> This fix introduces immutable sets of `PseudoClass` almost everywhere, as
>> they are rarely modified. These are re-used by caching them in a new class
>> `ImmutablePseudoClassSetsCache`.
>>
>> In order to make this work, `BitSet` had t
On Fri, 9 Jun 2023 12:45:02 GMT, John Hendrikx wrote:
>> This fix introduces immutable sets of `PseudoClass` almost everywhere, as
>> they are rarely modified. These are re-used by caching them in a new class
>> `ImmutablePseudoClassSetsCache`.
>>
>> In order to make this work, `BitSet` had t
On Thu, 3 Aug 2023 15:24:40 GMT, Marius Hanl wrote:
>> There is an `@Override` annotation, javadoc will do the right thing.
>
> So it is only needed when we want to add something to the javadoc coming from
> the superclass?
Yes, that's correct. `@inheritDoc` is only needed when you want to add
On Thu, 3 Aug 2023 15:22:39 GMT, Marius Hanl wrote:
>> John Hendrikx has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 16 commits:
>>
>> - Merge branch 'master' of https://git.openjdk.org/jfx into
>>feature/immutable-pseudoclas
On Thu, 3 Aug 2023 15:18:51 GMT, John Hendrikx wrote:
>> modules/javafx.graphics/src/main/java/com/sun/javafx/css/BitSet.java line
>> 263:
>>
>>> 261:
>>> 262:
>>> 263: /** {@inheritDoc} */
>>
>> Isn't this still needed?
>
> There is an `@Override` annotation, javadoc will do the right t
On Fri, 9 Jun 2023 12:45:02 GMT, John Hendrikx wrote:
>> This fix introduces immutable sets of `PseudoClass` almost everywhere, as
>> they are rarely modified. These are re-used by caching them in a new class
>> `ImmutablePseudoClassSetsCache`.
>>
>> In order to make this work, `BitSet` had t
On Thu, 3 Aug 2023 15:15:57 GMT, Marius Hanl wrote:
>> John Hendrikx has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 16 commits:
>>
>> - Merge branch 'master' of https://git.openjdk.org/jfx into
>>feature/immutable-pseudoclas
On Fri, 9 Jun 2023 12:45:02 GMT, John Hendrikx wrote:
>> This fix introduces immutable sets of `PseudoClass` almost everywhere, as
>> they are rarely modified. These are re-used by caching them in a new class
>> `ImmutablePseudoClassSetsCache`.
>>
>> In order to make this work, `BitSet` had t
On Wed, 5 Jul 2023 22:14:38 GMT, John Hendrikx wrote:
>> we can even consider to calculate it lazily, if we want to take this approach
>
> These sets are still created on demand, just as often as before, the main
> change is that there will no longer be thousands of exact duplicate sets in
> me
On Wed, 5 Jul 2023 20:47:20 GMT, Michael Strauß wrote:
>> John Hendrikx has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 16 commits:
>>
>> - Merge branch 'master' of https://git.openjdk.org/jfx into
>>feature/immutable-pseudoc
On Wed, 5 Jul 2023 20:56:51 GMT, Marius Hanl wrote:
>> modules/javafx.graphics/src/main/java/com/sun/javafx/css/ImmutablePseudoClassSetsCache.java
>> line 57:
>>
>>> 55: }
>>> 56:
>>> 57: Set copy = Set.copyOf(pseudoClasses);
>>
>> The set that is returned from this method wil
On Wed, 5 Jul 2023 20:52:28 GMT, Michael Strauß wrote:
>> John Hendrikx has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 16 commits:
>>
>> - Merge branch 'master' of https://git.openjdk.org/jfx into
>>feature/immutable-pseudoc
On Fri, 9 Jun 2023 12:45:02 GMT, John Hendrikx wrote:
>> This fix introduces immutable sets of `PseudoClass` almost everywhere, as
>> they are rarely modified. These are re-used by caching them in a new class
>> `ImmutablePseudoClassSetsCache`.
>>
>> In order to make this work, `BitSet` had t
40 matches
Mail list logo
