Notepad++ v8.7.6 released
-
@donho said in Notepad++ v8.7.6 released:
The change is for enhancing large files’ syntax highlighting performance:
https://github.com/notepad-plus-plus/notepad-plus-plus/commit/de9ffd2ea8507d033f7f111d8b48762f7d3b9436Since it’s for improving the performance & not all the plugins use such notifications, it won’t be reversed. Instead, I will add a new message NPPM_ADDSCINTILLANOTIFS for adding the notifications that plugins can add what they need, just after recieving NPPN_READY
[…]
What do you think?
I think it’s better than just removing functionality, though it could still break existing plugins if their authors are not active and able to keep up with changes. At least it would provide a quick way to restore compatibility when authors are available.
How will this work in connection with NPPN_GLOBALMODIFIED? Will SCN_MODIFIED messages requested by NPPM_ADDSCINTILLANOTIFS still be suppressed during Replace All operations?
Another thought has come to me, but the implications are unpleasant.
Columns++ only uses Scintilla notifications when implementing elastic tabstops, which can be enabled or disabled per document. By default, even after enabling it for one file, it is disabled when loading another file that is over 1000 KB or 5000 lines.
If excluding those notifications really does improve Notepad++ performance for large files, anyone with Columns++ installed would sacrifice that gain, even on files for which they were not using elastic tabstops (which is more likely to be the case with large files) — even those who never use that feature at all. Other plugins might face similar circumstances.
At present, I have no suggestion as to how that could be avoided in practice, though. For my plugin, since the loss of SC_MOD_BEFOREDELETE will cause degradation in responsiveness, rather than outright inaccurate behavior — and there may be another, more complex way to restore the responsiveness — I will have some design considerations to weigh with or without NPPM_ADDSCINTILLANOTIFS.
-
@Coises said in Notepad++ v8.7.6 released:
How will this work in connection with NPPN_GLOBALMODIFIED? Will SCN_MODIFIED messages requested by NPPM_ADDSCINTILLANOTIFS still be suppressed during Replace All operations?
What’s needed is a complelmentary set of
*_GET_*and*_SET_*messages like Scintilla has for most properties.Proper usage would be sending the
*_GET_*message first, masking the return value for the desired flag(s), then restoring the flags by sending the*_SET_*message with the old value after the plugin is finished modifiying them. -
I recommend that one of the plugin authors involved in this discussion create an issue at this point (referencing this topic, of course): now that the problem is defined, there appears to need to be more technical discussion as to how a fix might be implemented, and that’s better suited to an Issue than the Forum.
(The technical details might dissuade other users from posting other regressions in this discussion, and if the notification-discussion continues after someone does post another regression, their post may get lost in between posts about this issue.)
-
Please test this PR WITH your modified plugins:
https://github.com/notepad-plus-plus/notepad-plus-plus/pull/16120Please let me know if the PR works for you.
-
@Coises said in Notepad++ v8.7.6 released:
though it could still break existing plugins if their authors are not active and able to keep up with changes.
I think it’s not reasonable to stop evolving a project like Notepad++ due to unmaintained plugins.
How will this work in connection with NPPN_GLOBALMODIFIED ? Will SCN_MODIFIED messages requested by NPPM_ADDSCINTILLANOTIFS still be suppressed during Replace All operations?
There’s no connection between NPPM_ADDSCINTILLANOTIFS & NPPN_GLOBALMODIFIED. They work separately.
If excluding those notifications really does improve Notepad++ performance for large files, anyone with Columns++ installed would sacrifice that gain, even on files for which they were not using elastic tabstops (which is more likely to be the case with large files) — even those who never use that feature at all. Other plugins might face similar circumstances.
There are several factors come together to enhance large files’ syntax highlighting performance. Furthermore, it’s about only the performance of switching-in a large file.
So I think it’s OK if one of factors (Scintilla notifications) is turned ON by plugins. The consequence is not significative IMO. -
@PeterJones
A regression issue has been created:
https://github.com/notepad-plus-plus/notepad-plus-plus/issues/16121 -
@donho said in Notepad++ v8.7.6 released:
There are several factors come together to enhance large files’ syntax highlighting performance. Furthermore, it’s about only the performance of switching-in a large file.
So I think it’s OK if one of factors (Scintilla notifications) is turned ON by plugins. The consequence is not significative IMO.At the risk of appearing argumentative… then why disable those events in the first place?
I cannot claim to have completely followed all the discussion behind the change to improve handling in large files, but it looks like the change to the SCN_MODIFIED event mask was based on this:
https://github.com/notepad-plus-plus/notepad-plus-plus/pull/15981#issuecomment-2563388005
I think the improvements to “the performance of switching-in a large file” were connected to turning off all SCN_MODIFIED events during loading. It looks like a note about restoring the environment after loading that said:
Set MODEVENTMASK_ON with SC_MOD_INSERTTEXT | SC_MOD_DELETETEXT only: Adding additional flags generates excessive and unnecessary event messages, degrading performance.
was the basis for changing the default event mask. I suspect this might have been a hasty remark, perhaps not considering that just because these events are unnecessary to Notepad++ itself does not mean that they are unnecessary to plugins. (The example given for overhead that the commenter claims could be skipped by suppressing unnecessary events is for an event, SC_MOD_CHANGEINDICATOR, that is included in the new defaults and won’t be disabled — so apparently it was necessary. In any case, if Notepad++ is doing unnecessary things in response to SCN_MODIFIED events, the sensible thing is to verify that they are unnecessary and remove that code. I suspect there isn’t a lot of code that was added for no reason at all… this logic seems suspect to me.)
I’m questioning whether there is any real evidence that changing which SCN_MODIFIED events are normally enabled (completely separate from temporarily disabling all events during certain activities internal to Notepad++) gains anything. And if anything, enough to warrant the disruption for existing plugins and creation of a new NPPM message?
-
@Coises said in Notepad++ v8.7.6 released:
Set MODEVENTMASK_ON with SC_MOD_INSERTTEXT | SC_MOD_DELETETEXT only: Adding additional flags generates excessive and unnecessary event messages, degrading performance.
More events, less performance.
In absolute terms, it does make sense to me.was the basis for changing the default event mask. I suspect this might have been a hasty remark, perhaps not considering that just because these events are unnecessary to Notepad++ itself does not mean that they are unnecessary to plugins. (The example given for overhead that the commenter claims could be skipped by suppressing unnecessary events is for an event, SC_MOD_CHANGEINDICATOR, that is included in the new defaults and won’t be disabled — so apparently it was necessary. In any case, if Notepad++ is doing unnecessary things in response to SCN_MODIFIED events, the sensible thing is to verify that they are unnecessary and remove that code. I suspect there isn’t a lot of code that was added for no reason at all… this logic seems suspect to me.)
It’s true while excluding unnecessary events for Notepad++, the needs of plugins were overlooked unintentionally.
But since now there’s a new API for plugins to add the events they want to listen to, from my perspective, it might be a better way - especially since the plugins in question are not installed by all users.I’m questioning whether there is any real evidence that changing which SCN_MODIFIED events are normally enabled (completely separate from temporarily disabling all events during certain activities internal to Notepad++) gains anything. And if anything, enough to warrant the disruption for existing plugins and creation of a new NPPM message?
That’s a good point. I will test it from my side.
-
The PR2 which reverts the excluded SCN_MODIFIED events is here:
https://github.com/notepad-plus-plus/notepad-plus-plus/pull/16128I have done the test with a huge XML file (400 MB) to compare between the PR to revert & the PR1 with new API
NPPM_ADDSCNMODIFIEDFLAGS, by doing the following steps:- Open the huge XML and quit Notepad++ - so the huge XML file stays in the session.
- Reopen Notepad++, make sure the 1st line is on the top. Immediatly hit
Ctrl-PageDown. And use the timer to count the time to need to show the bottom text with syntax highlighting.
Here’s my result:
-
PR2 with WRAP_ON: 39.72 - 39.72 sec to get the bottom with syntax highlighting.
-
PR2 with WRAP_OFF: 38 - the bottom line is reached immediately, and it takes 38 seconds to have syntax highlighting.
-
PR1 with WRAP_ON: 12.63 - 12.63 sec to get the bottom with syntax highlighting.
-
PR1 with WRAP_OFF: 12.88 - the bottom line is reached immediately, and it takes 12.88 seconds to have syntax highlighting.
Conclusion, for the sake of performance, let’s use the PR1. And let plugins add the events they need for not overloading Notepad++ performance.
-
@Coises said in Notepad++ v8.7.6 released:
At the risk of appearing argumentative… then why disable those events in the first place?
I remember e.g. this performance hint from the Scintilla author:
“Notepad++, unlike SciTE, leaves most notifications enabled so each replace operation will send 5 notifications [BeforeDelete, DeleteText, InsertCheck, BeforeInsert, InsertText]. Command events are also enabled which sends a WM_COMMAND:EN_CHANGE for each notification. These may not all be needed …”
And here is my similar thought:
“…instead of unnecessarily burdening all the present plugins with unsolicited notifications (for most of them), it would be better if plugins that want something like this, actively request (registered somehow?) that info being sent only to them by the N++.”
