Re: [PATCH] Simplification of TreeMap

[Michael Kuhlmann]

Hi Sergey,

I was also wondering why TreeMap doesn't just use a default comparator
and always checks for null instead.

The problem with your patch is that deserialized TreeMap instances which
were serialized from a previous version would still have the comparator
set to null, thus resulting in a NPE after your patch. And you can't
easily fix this because comparator is a final field.

Best,
Michael


On 04.09.2018 21:14, Сергей Цыпанов wrote:
> Hi,
> 
> currently (latest code of JDK 11) an instance of TreeMap created with no-arg 
> contructor has nullable comparator i.e. utilizes no comparator.
> 
> As it comes from the code, a TreeMap created with nullable comparator works 
> exactly as a TreeMap created with comparator provided by 
> Comparator.naturalOrder(). This is also explicitly specifid in Javadoc.
> 
> I propose to initialize default comparator of TreeMap with instance of 
> Comparator returned by Comparator.naturalOrder() instead of null.
> This allows to remove the code responsible for handling nullable comparator, 
> e. g. TreeMap::getEntryUsingComparator can be completely removed in favour of 
> TreeMap::getEntry.
> Similar simplification available for TreeMap::put, TreeMap::compare, 
> EntrySpliterator::getComparator.
> 
> I've prepared a patch for this.
> The patch contains both described major change and some tiny clean-ups e. g. 
> utilization of Objects::requireNonNull where appropriate and Objects::equals 
> instead of hand-written TreeMap::valEquals.
> 
> TreeMapTest is green after my changes.
> 
> Regards,
> Sergey Tsypanov
> 
> 

Re: [PATCH] Simplification of TreeMap

[Michael Kuhlmann]

Hi Sergey and Sergey ;),

that's totally correct! Serialization really is a beast sometimes, you
have to take care that instances that have been serialized even with
Java 1.2 still get deserialized correctly in the current version.

So it's not sufficient to only look at the constructors when the class
is Serializable. In the case of TreeMap, the only solutions I see are
either making 'comparator' mutable and setting it in readObject(), or
again always check for null values when accessing it - which would make
your idea rather useless. As Martin already mentioned, "if it's not
broken, don't fix it" - either option is not worth the change.

What would make sense is to remove valEquals() in favor of
Objects::equals, so at least a minor patch would remain. ;)

-Michael


Am 05.09.2018 um 21:22 schrieb Sergey:
> Hi Sergey,
> 
> Michael might correct me if I’ve missed something, but 
> problem with your test case is that you’re serializing already 
> patched version. That makes sense if you want to test current 
> behavior. However the case you truly want to test is how your
> patched TreeMap deserializes pre-patched TreeMaps.
> 
> What you have currently just tests if patched map could be
> deserialized without any problems.
> 
> Cheers,
> su -
> 
> On Wed, 5 Sep 2018 at 20:30, Сергей Цыпанов  > wrote:
> 
> Hi Michael,
> 
> thanks for pointing out this serialization concern, I didn't think
> about it at all.
> 
> I've wrote a simple test for serialization of patched TreeMap and it
> works without errors for both no-args constructor and constructor
> with comparator:
> 
> public class TreeMapSerialization {
>     public static void main(String[] args) throws Exception {
>         TreeMap serialized = new
> TreeMap<>(Comparator.reverseOrder());
>         serialized.put(1, "1");
>         serialized.put(2, "2");
> 
>         ByteArrayOutputStream baos = new ByteArrayOutputStream();
>         ObjectOutputStream oos = new ObjectOutputStream(baos);
> 
>         oos.writeObject(serialized);
>         oos.flush();
>         baos.flush();
>         oos.close();
>         baos.close();
> 
>         ByteArrayInputStream bais = new
> ByteArrayInputStream(baos.toByteArray());
>         ObjectInputStream ois = new ObjectInputStream(bais);
> 
>         TreeMap deserialized = (TreeMap String>) ois.readObject();
>         deserialized.put(3, "3");
> 
>         System.out.println(deserialized);
>     }
> }
> 
> 
> I hope I don't miss anything, so there shouldn't be any
> serialization issues.
> 
> Regards,
> Sergey Tsypanov
> 

Re: 'Find' method for Iterable

[Michael Kuhlmann]

Hi Nir,

at first I thought "Wow, it would be really cool to have that method in 
Iterable! Why isn't it there already?"


But after thinking about it, I'm now convinced that it would be a bad 
idea. Because it extends the scope of this small, tiny Iterable 
interface to something bigger which it shouldn't be.


When some class implements Iterable, it just says "you can iterate over 
my something which I call elements". Nothing more. Now when Iterable 
implements find() by default, then automatically all classes which just 
want to enable users to iterate over elements also tell them that there 
can be something useful found in these elements, which is not 
necessarily the case.


For example, BitSet could immplements Iterable. That doesn't 
make much practical sense, but from the definition of a BitSet it's 
understandable: A BitSet can be seen as a set of integer values, why 
shouldn't someone iterate over them. But now, when you add find() to 
Iterable, it immediately tells users: Hey, you can find integers in me, 
and when you found one, you get it returned. Which is beyond the use 
case of a BitSet.


forEach() is different, because forEach just iterates over the elements 
and nothing more, which is in the scope of an Iterable.


A second argument against adding find() is that such a generic method 
could conflict with more specialized methods in subinterfaces or 
classes. I like the idea of having indexOf(Predicate) in List 
interface, but having both of them would be redundant.


And a third argument is that it can break existing code. An implementor 
of Iterable could already define a find() method, but return the index 
of the element instead of the element itself, or throw some checked 
exception. This code wouldn't compile any more.


So while the idea of having find() in Iterable is great, the arguments 
against are heavier from my point of view.


-Michael


On 9/16/20 11:36 PM, Nir Lisker wrote:

I don't see a reason to put it Collection when it extends Iterable anyway,
and the method just requires iteration. As for execution time, true, it's
faster, but Map uses a lot more memory, so it's a tradeoff. For smaller
lists, linear time is acceptable. Currently I'm using Maps actually, but I
find that when there are many small maps, having many small lists is better
for memory and the search time is similar. Additionally, a Map works only
for searching by 1 key, but with a Collection/Iterable I can search by any
property, and we're not about to use a Map for every property. So, overall,
I don't think Map is a competitor in this market. It's also possible to
specify that the complexity is linear in an @implNote to avoid surprises.

- Nir

On Wed, Sep 16, 2020 at 11:59 PM Remi Forax  wrote:


- Mail original -

De: "Nir Lisker" 
À: "core-libs-dev" 
Envoyé: Lundi 14 Septembre 2020 20:56:27
Objet: 'Find' method for Iterable



Hi,

This has probably been brought up at some point. When we need to find an
item in a collection based on its properties, we can either do it in a
loop, testing each item, or in a stream with filter and findFirst/Any.

I would think that a method in Iterable be useful, along the lines of:

public  Optional find(Predicate condition) {
Objects.requireNonNull(condition);
for (T t : this) {
 if (condition.test(t)) {
 return Optional.of(t);
}
}
return Optional.empty();
}

With usage:

list.find(person -> person.id == 123456);

There are a few issues with the method here such as t being null in
null-friendly collections and the lack of bound generic types, but this
example is just used to explain the intention.

It will be an alternative to

list.stream().filter(person -> person.id == 123456).findAny/First()
(depending on if the collection is ordered or not)

which doesn't create a stream, similar to Iterable#forEach vs
Stream#forEach.

Maybe with pattern matching this would become more appetizing.


During the development of Java 8, we first tried to use Iterator/Iterable
instead of using a novel interface Stream.
But a Stream cleanly separate the lazy side effect free API from the
mutable one (Collection) and can be optimized better by the VM (it's a push
API instead of being a pull API).

The other question is why there is no method find() on Collection, i
believe it's because while find() is ok for any DB API, find() is dangerous
on a Collection because the execution time is linear, so people may use it
instead of using a Map.



- Nir


Rémi

Re: RFR: 8263668: Update java.time to use instanceof pattern variable

[Michael Kuhlmann]

On 3/24/21 12:09 PM, Rémi Forax wrote:

On Wed, 24 Mar 2021 09:56:16 GMT, Patrick Concannon  
wrote:


Hi,

Could someone please review my code for updating the code in the `java.time` 
package to make use of the `instanceof` pattern variable?

Kind regards,
Patrick


src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 
168:


166: private static final TemporalQuery QUERY_REGION_ONLY = (temporal) 
-> {
167: ZoneId zone = temporal.query(TemporalQueries.zoneId());
168: return (zone != null && (!(zone instanceof ZoneOffset)) ? zone : 
null);


i find this code hard to read
`return (zone != null && (!(zone instanceof ZoneOffset))) ? zone : null;`
seems better`


The whole null check is not necessary.

`return zone instanceof ZoneOffset ? null : zone;`


-

PR: https://git.openjdk.java.net/jdk/pull/3170

Re: System.getEnv(String name, String def)

[Michael Kuhlmann]

Hi Rémi,

I don't want to be pedantic, but I see this as an anti-pattern. You 
would create an Optional just to immediately call orElse() on it. That's 
not how Optional should be used. (But you know that.)


It's described in Recipe 12 of this Java Magazine article, for instance: 
https://blogs.oracle.com/javamagazine/12-recipes-for-using-the-optional-class-as-its-meant-to-be-used


Best,
Michael

On 2/15/21 3:09 PM, Remi Forax wrote:

Hi Loic,
You can use Optional.OfNullable() which is a kind of the general bridge between 
the nullable world and the non-nullable one.

   var fooOptional = Optional.ofNullable(System.getenv("FOO"));
   var fooValue = fooOptional.orElse(defaultValue);

regards,
Rémi Forax

- Mail original -

De: "Loïc MATHIEU" 
À: "core-libs-dev" 
Envoyé: Lundi 15 Février 2021 14:59:42
Objet: System.getEnv(String name, String def)



Hello,

I wonder if there has already been some discussion to provide
a System.getEnv(String name, String def) method that allows to return a
default value in case the env variable didn't exist.

When using system properties instead of env variable, we do have a
System.getProperty(String key, String def) variant.

Stating the JavaDoc of System.getEnv():
*System properties and environment variables are both conceptually mappings
between names and values*

So if system properties and environment variables are similar concepts,
they should provide the same functionalities right ?

This would be very convenient as more and more people rely on
environment variables these days to configure their applications.

Regards,

Loïc

Re: LambdaMetafactory requires full privilege access, but doesn't seem to actually restrict functionality

[Michael Kuhlmann]

Hi Steven!

Sorry for coming back so lately; the main reason is that I can't answer 
your question. The issue you are describing indeed is a bit weird, and I 
can only speculate that it's like this because LambdaMetaFactory was 
mainly designed to be used in bootstrap methods for invokedynamic calls. 
So the checks are probably more restrictive than it should be when being 
used in standard Java code.


But I'm curious about your motivation on using Lambdas at all. Of 
course, I don't have detailed insights and may miss some important 
context. Maybe my idea is completely wrong and nonsense; in that case I 
apologize for being so intrusive. But I was already wondering in the 
past why Jackson is using bytecode generation when simple MethodHandle 
combinations would be much easier to use.


Lambdas like Function and BiConsumer seem to be a bad choice for me. 
There are two main downsides to it:
- LambdaMetaFactory only accepts DirectMethodHandles. At least it allows 
simple type conversions as autoboxing and hierarchical castings, but 
that's it. Even a simple conversion from a String input (what you 
probably have when parsing a JSON structure) to a numeric value is 
impossible to integrate.
- I assume you plan to iterate over the generated lambdas. In this case, 
Hotspot won't be able to optimize much because each Function/BiConsumer 
has a different implementation. It's impossible to predict much or even 
inline much. I would expect the performance to be much lower than the 
current one.


Why not just concatenating MethodHandles directly? The strategy could be 
as the following:
- Use filterArguments to make the getter's return type and the setter's 
argument convertable
- Use filterArguments again to directly inject the getter into the 
setter's argument; so getter and setter will merge into one "transfer" 
handle
- Use foldArguments to consecutively perform the transfer handle for 
each attribute; the resulting handle will transfer all attributes from 
source to destination
- Use foldArgument again with MethodHandles::identity to let this 
transfer handle return its argument
- And again, use foldArguments with the constructor handle to convert 
the resulting handle into a simple one-argument-handle expecting the 
source as a parameter and the newly creation target element as the 
return value.
- Use such a handle also for type conversion of nested Pojos (for 
to-many relations, you have to be creative).


As a result, you will have only one simple MethodHandle to convert the 
whole structure at once.


As a background, back in times I was writing a library to convert Solr 
result objects to standard Pojos. The main logic was to convert Maps to 
nested Pojos and vice versa. I used exactly that strategy. It as fun, 
much better than solving Sudoku ;), and it worked well even with Java 8 
features. With the new loop and iterate handles introduced with Java 9, 
you have even more options.


Really, I'm curious if this could be an approach for Jackson. Or if not, 
what could be the obstacles.




On 1/12/22 9:56 PM, Steven Schlansker wrote:

Hi core-libs-dev,

I am maintaining a module for the popular Jackson JSON library that attempts to 
simplify code-generation code without losing performance.
Long ago, it was a huge win to code-generate custom getter / setter / field 
accessors rather than use core reflection. Now, the gap is closing a lot with 
MethodHandles, but there still seems to be some benefit.

The previous approach used for code generation relied on the CGLib + ASM 
libraries, which as I am sure you know leads to horrible-to-maintain code since 
you essentially write bytecode directly.
Feature development basically stopped because writing out long chains of 
`visitVarInsn(ASTORE, 3)` and the like scares off most contributors, myself 
included.

As an experiment, I started to port the custom class generation logic to use 
LambdaMetafactory. The idea is to use the factory to generate `Function` 
getter and `BiConsumer` setter implementations.
Then, use those during (de)serialization to access or set data.  Eventually 
hopefully the JVM will inline, removing all (?) reflection overhead.

The invocation looks like:
var lookup = MethodHandles.privateLookupIn(targetClass, 
MethodHandles.lookup()); // allow non-public access
var getter = lookup.unreflect(someGetterMethod);
LambdaMetafactory.metafactory(
   lookup,
   "apply",
   methodType(Function.class),
   methodType(Object.class, Object.class),
   getter,
   getter.type())

This works well for classes from the same classloader. However, once you try to 
generate lambdas with implementations loaded from a different classloader, you 
run into a check in the AbstractValidatingLambdaMetafactory constructor:

if (!caller.hasFullPrivilegeAccess()) {
   throw new LambdaConversionException(String.format(
 "Invalid caller: %s",
 caller.lookupClass().getName()));
}

The `privateLookupIn` call seems to drop MODULE privilege access when looking 

Re: LambdaMetafactory requires full privilege access, but doesn't seem to actually restrict functionality

[Michael Kuhlmann]

Hi Steven!

On 1/19/22 2:23 AM, Steven Schlansker wrote:

Interestingly, that wasn't what I found.  At least as of Java 14, the 
Metafactory generation approach (as limited as my first pass was) was 
competitive with CGLib hand-generated code in many (but not all) cases, and 
significantly faster than Method.invoke:
https://github.com/FasterXML/jackson-modules-base/tree/2.14/blackbird/benchmarks
Unfortunately I don't have a MethodHandle.invokeExact comparison in that 
benchmark. I should go back and finish that work, and re-run it all on 17. If 
we can call non-static MethodHandles with little overhead, then any reason to 
use the Metafactory here goes away.


Very interesting! Thank you for sharing it.
It's true that for non-static MethodHandles it's a similar problem as 
for generic lambdas: Hotspot can't easily inline the call. I wouldn't 
have expected that MethodHandles are even slower than lambas, but 
honestly I never measured it.





Really, I'm curious if this could be an approach for Jackson. Or if not, what 
could be the obstacles.


I think the problem here is just slightly different: your approach will copy 
from one Java object to another Java object, but what we are doing is reacting 
to a token stream and trying to bind it to Java objects with as little overhead 
as possible.


So I assume your token handler is getting the caller (be it a handle, a 
lambda or whatever) from some key-value store (maybe a Map) and 
injecting the value into it.


What you can try is using MethodHandles::exactInvoker combined with 
foldArguments, where the token-to-handle mapper is already injected. 
The handle would just expect the token key and the value as arguments. 
Maybe this would perform better then the non-static setter handles.


I did this for another use case where the target handle was the 
dynamicInvoker of a MutableCallSite. The handle then lazily generated 
its own final handle on the first call and set it into its CallSite, so 
that all subsequent calls ran directly into the generated handle. It was 
working great and a lot of fun to code, but again, I never measured the 
performance in detail.


I wish you good luck!
Michael

Re: fast way to infer caller

[Michael Kuhlmann]

On 4/7/22 19:27, Kasper Nielsen wrote:


nope, see my previous mail to Ceki, the VM is cheating here if it can
inline the call to MethodHandles.lookup()



Does how the VM cheats really matter? The fact is that the code in the JDK
can
get the calling class and implement something like MethodHandles.lookup() so
it takes ~3 ns. If you implemented something like a lookup class as a normal
user your best bet would be StackWalker.GetCallingClass() and you would end
up with something that is at least 2 magnitudes slower. That is probably not
an issue for most use cases. But for some, it might be a bit of a steep
cost.

/Kasper


Hi Kasper,

sorry to jump in here as an uninvolved onlooker, but I can't resist.
I really don't see why this should matter. Getting the caller class is a 
rare edge case that you just do in exceptional situations; most often 
it's more for debugging or something.


What users really are interested in is high performance for standard 
cases. Implementing a specific optimization into Hotspot to gain few 
milliseconds is the least thing I expect from the JVM developers.


I also don't understand why someone should instantiate a logger during 
performance critical procedures. In more than 20 years of Java 
development, I've never seen the need to create a logger on the fly. 
They are *always* assigned to final static variables, or at least to 
predefined pools. Everything else would be just wrong: To instantiate a 
logger, you have to fetch at least the log level definition from some 
configuration source, and this can never be fast. At least not that 
we're talking about nanoseconds here.


All logging implementations I know of (and all that make sense) are 
highly optimized on log throughput; this can only be achieved by 
preprocessing during initialization, why this is slow. But that doesn't 
matter, because, as said, you should anyway create logger instances 
beforehand.


Sorry for the rant, but I really don't see the use case here.

Re: fast way to infer caller

[Michael Kuhlmann]

Good morning!

On 4/7/22 23:59, Kasper Nielsen wrote:

Hi Michael,

I don't really have an opinion on how you obtain a logger. But one 
particular

use-case I've had was that I would like to perform some access checks based
on the module of the caller. Something similar to how Panama checks for
native access:

Reflection.ensureNativeAccess(Reflection.getCallerClass());


Hmh, but wouldn't it be sufficient to perform these checks at 
initialization time?


On 4/7/22 21:01, Ralph Goers wrote:
> In Log4j’s case there are two places that accessing the stack comes 
into play:

> 1. Obtaining a Logger .
> 2. Logging an event and including the caller’s location information.
>
> To be honest, what sucks for Logging frameworks is that we really want
> access to the location information at compile time such that the overhead
> to include it would be zero. I’ve never understood why Java doesn’t 
include it.


That I totally agree! Having the exact line number in the log statement 
would be a huge win.


But to be honest, Reflection::getCallerClass wouldn't help either.

I agree that such an inlined statement would be a huge win.

But to also put this into relation: From my opinion, logging frameworks 
should be most performant in the cases where they are *not* logging. Or 
in those cases where you would log a lot even in production environment 
- like for financial calculations - the exact line number isn't that 
relevant any more.


But that's just my opinion.

-Michael

Re: core-libs-dev Digest Vol 180 Issue 174 Topic 1

[Michael Kuhlmann]

On 4/20/22 10:15, sminervini.prism wrote:
> To the Java Community Process, and OpenJDK community,
>
> A number of us have been waiting on serious consideration for 
apprehension of

>
> core-libs-dev Digest Vol 180 Issue 174 Topic 1
>
> since we have posted, but not received an addressal and apprehending 
response

>
> in some time. We know that our posting comes soon after Easter. 
Still, we have

>
> been wondering what the internal state of discussion is on the 
aforementioned

>
> Topic 1, and are hoping that it can be apprehended, certainly in the 
compatible

>
> manner it discusses, to gain the advantages discussed, while removing
>
> disadvantages and bringing on board no real disadvantages, for the
>
> Java OpenJDK and the Java Community at the more official level.
>
> We wait with bated breath for a reply in future core-libs-dev
>
> digests for a positive, active reply!
>
> Sent with [ProtonMail](https://protonmail.com/) secure email.


Heeeh I don't understand a single word.
What is this "core-libs-dev Digest Vol 180 Issue 174 Topic 1" stuff you 
are talking about? It would help to refer to a concrete message instead 
of some digest number.


The only thing I can remember is that you were spamming the list with at 
least three identical postings which has been answered by Andrew right 
after the first message already.


If this is what you're asking for, here is what he wrote:

On 4/16/22 09:04, sminervini.prism wrote:
> It is still the case that I and others need Java floating point and 
StrictMath

> (the key double type calculator class) to be repaired.

If you are going to pursue this line of reasoning, you must be aware
of some things.

Firstly, it is not possible to change Java's core floating-point
arithmetic in a compatible way. We certainly will not adopt decimal
floating-point for Java's core arithmetic.

Secondly, Java should not be your focus. We will not change Java's
arithmetic in a way that is incompatible with other languages or
which makes Java slower on existing hardware.

You must read and fully understand this before you go any further. It
will require a lot of study:

https://docs.oracle.com/cd/E19957-01/806-3568/ncg_goldberg.html

> May the Java Community Process reconsider the present floating point
> operations and method calls situation, based on an imperfect
> standard and improper workaround, and provide corrected, defaulting
> or specifying-compatible waya for Java floating point arithmetic and
> Calculator class method results to always cohere in their ranges,
> without denormal and pronormal inclusion?

In a word, no. That possibility is so unlikely that it is not worthy
of consideration.

--
Andrew Haley  (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. 
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671