perf: improve DSPLaunchDelegate debug adapter monitor performance #1080
Conversation
| @@ -323,16 +324,14 @@ public int read() throws IOException { | |||
| return -1; | |||
| } | |||
| }; | |||
| DSPLaunchDelegateLaunchBuilder finalBuilder = builder; | |||
| final var consoleEncoding = builder.launch.getAttribute(DebugPlugin.ATTR_CONSOLE_ENCODING); | |||
| final var consoleCharset = consoleEncoding == null // | |||
There was a problem hiding this comment.
could you add to the PR description why are you adding this two calls to Charset instead of just passing consoleEncoding?
There was a problem hiding this comment.
consoleEncoding might return null or what do you mean? I just moved the instruction out of the listener where it is unnecessarily reevaluated on each callback
There was a problem hiding this comment.
Previously the value returned by builder.launch.getAttribute(DebugPlugin.ATTR_CONSOLE_ENCODING) was passed to text.getBytes as is (just casting to NonNull).
Now if the value is null we map it to a default charset. Is that an intentional change? If so I think it would be worth putting in the commit and PR messages.
There was a problem hiding this comment.
Because getBytes(String) would first lookup the CharSet object. If we obtain the Charset before and directly pass it to getBytes() we save that lookup on each listener event.
There was a problem hiding this comment.
Thanks for the explanation. To be equivalent as before, we should throw a NPE if builder.launch.getAttribute(DebugPlugin.ATTR_CONSOLE_ENCODING), otherwise we are changing the behavior, are we not?
I am fine changing the behaviour, but I think it should be added to the commit an PR messages.
There was a problem hiding this comment.
I actually think falling back to the default charset is more sensible than throwing an exception if the encoding is not overwritten in the launch config.
There was a problem hiding this comment.
Yes, me too.
Could you amend the commit message and add "If the console encoding was not specified in the launch config it now falls back to the JVM's default encoding instead of throwing an exception." there as well?
Then I am good
This commit replaces `Collections.synchronizedList(new LinkedList<Byte>())` with `new ConcurrentLinkedQueue<Byte>()` and only determines the console encoding once and not on every text output. If the console encoding was not specified in the launch config it now falls back to the JVM's default encoding instead of throwing an exception.
sebthom
force-pushed
the
dsp-launch-delegate
branch
from
4e97c95
to
55f0526
Compare
This commit replaces
Collections.synchronizedList(new LinkedList<Byte>())withnew ConcurrentLinkedQueue<Byte>()and only determines the console encoding once and not on every text output.If the console encoding was not specified in the launch config it now falls back to the JVM's default encoding instead of throwing an exception.