Menu
▾
▴
jedit-devel
|
From: Dale A. <da...@gr...> - 2009-09-29 21:03:13
|
In TokenMarker.java, there are these lines starting at line 292: 292: LineContext tempContext = context; 293: context = context.parent; 294: keywords = context.rules.getKeywords(); 295: boolean handled = handleRule(rule,true); 296: context = tempContext; 297: keywords = context.rules.getKeywords(); Does anyone know why the line at 294 is there at all? The value for 'keywords' is replaced in line 297 and it isn't used in between. I'm getting null pointer exceptions from line 294 fairly often. Just hitting the "Enter" button while typing in the text area causes it, then there is a beanshell error dialog and the cursor goes to the wrong place, there are painting issues, and so on. I've removed line 294 in my local copy, and things seem to be working better so far. I'm not familiar with this piece of code at all, so I don't know if context.rules on line 294 should ever be null. Thanks, Dale |
|
From: daniel j. <de...@gm...> - 2009-09-30 02:30:57
|
Just pressing enter causes exceptions for you? In svn head? On Tue, Sep 29, 2009 at 4:03 PM, Dale Anson <da...@gr...> wrote: > In TokenMarker.java, there are these lines starting at line 292: > > 292: LineContext tempContext = context; > 293: context = context.parent; > 294: keywords = context.rules.getKeywords(); > 295: boolean handled = handleRule(rule,true); > 296: context = tempContext; > 297: keywords = context.rules.getKeywords(); > > > Does anyone know why the line at 294 is there at all? The value for > 'keywords' is replaced in line 297 and it isn't used in between. I'm > getting null pointer exceptions from line 294 fairly often. Just > hitting the "Enter" button while typing in the text area causes it, > then there is a beanshell error dialog and the cursor goes to the > wrong place, there are painting issues, and so on. I've removed line > 294 in my local copy, and things seem to be working better so far. > I'm not familiar with this piece of code at all, so I don't know if > context.rules on line 294 should ever be null. > > Thanks, > > Dale > > ------------------------------------------------------------------------------ > Come build with us! The BlackBerry® Developer Conference in SF, CA > is the only developer event you need to attend this year. Jumpstart your > developing skills, take BlackBerry mobile applications to market and stay > ahead of the curve. Join us from November 9-12, 2009. Register now! > http://p.sf.net/sfu/devconf > -- > ----------------------------------------------- > jEdit Developers' List > jEd...@li... > https://lists.sourceforge.net/lists/listinfo/jedit-devel > -- Daniel Johnson de...@gm... |
|
From: Matthieu C. <cho...@gm...> - 2009-09-30 07:03:55
|
Hi, I don't know exactly, but I suppose those keywords are used in handleRule, don't you think ? 2009/9/29 Dale Anson <da...@gr...> > In TokenMarker.java, there are these lines starting at line 292: > > 292: LineContext tempContext = context; > 293: context = context.parent; > 294: keywords = context.rules.getKeywords(); > 295: boolean handled = handleRule(rule,true); > 296: context = tempContext; > 297: keywords = context.rules.getKeywords(); > > > Does anyone know why the line at 294 is there at all? The value for > 'keywords' is replaced in line 297 and it isn't used in between. I'm > getting null pointer exceptions from line 294 fairly often. Just > hitting the "Enter" button while typing in the text area causes it, > then there is a beanshell error dialog and the cursor goes to the > wrong place, there are painting issues, and so on. I've removed line > 294 in my local copy, and things seem to be working better so far. > I'm not familiar with this piece of code at all, so I don't know if > context.rules on line 294 should ever be null. > > Thanks, > > Dale > > > ------------------------------------------------------------------------------ > Come build with us! The BlackBerry® Developer Conference in SF, CA > is the only developer event you need to attend this year. Jumpstart your > developing skills, take BlackBerry mobile applications to market and stay > ahead of the curve. Join us from November 9-12, 2009. Register now! > http://p.sf.net/sfu/devconf > -- > ----------------------------------------------- > jEdit Developers' List > jEd...@li... > https://lists.sourceforge.net/lists/listinfo/jedit-devel > |
|
From: Shlomy R. <sre...@gm...> - 2009-09-30 07:19:32
|
Without knowing the code, just a brief look shows me this is a mode-specific issue that is probably related to delegation of mode attributes. So, in which mode does it happen? Maybe something's wrong with the mode file? Shlomy On Wed, Sep 30, 2009 at 9:03 AM, Matthieu Casanova <cho...@gm...> wrote: > Hi, I don't know exactly, but I suppose those keywords are used in > handleRule, don't you think ? > > 2009/9/29 Dale Anson <da...@gr...> >> >> In TokenMarker.java, there are these lines starting at line 292: >> >> 292: LineContext tempContext = context; >> 293: context = context.parent; >> 294: keywords = context.rules.getKeywords(); >> 295: boolean handled = handleRule(rule,true); >> 296: context = tempContext; >> 297: keywords = context.rules.getKeywords(); >> >> >> Does anyone know why the line at 294 is there at all? The value for >> 'keywords' is replaced in line 297 and it isn't used in between. I'm >> getting null pointer exceptions from line 294 fairly often. Just >> hitting the "Enter" button while typing in the text area causes it, >> then there is a beanshell error dialog and the cursor goes to the >> wrong place, there are painting issues, and so on. I've removed line >> 294 in my local copy, and things seem to be working better so far. >> I'm not familiar with this piece of code at all, so I don't know if >> context.rules on line 294 should ever be null. >> >> Thanks, >> >> Dale >> >> >> ------------------------------------------------------------------------------ >> Come build with us! The BlackBerry® Developer Conference in SF, CA >> is the only developer event you need to attend this year. Jumpstart your >> developing skills, take BlackBerry mobile applications to market and stay >> ahead of the curve. Join us from November 9-12, 2009. Register >> now! >> http://p.sf.net/sfu/devconf >> -- >> ----------------------------------------------- >> jEdit Developers' List >> jEd...@li... >> https://lists.sourceforge.net/lists/listinfo/jedit-devel > > > ------------------------------------------------------------------------------ > Come build with us! The BlackBerry® Developer Conference in SF, CA > is the only developer event you need to attend this year. Jumpstart your > developing skills, take BlackBerry mobile applications to market and stay > ahead of the curve. Join us from November 9-12, 2009. Register now! > http://p.sf.net/sfu/devconf > -- > ----------------------------------------------- > jEdit Developers' List > jEd...@li... > https://lists.sourceforge.net/lists/listinfo/jedit-devel > > |
|
From: Dale A. <da...@gr...> - 2009-09-30 15:06:44
|
This happens when editing java files. I am using head of svn, my current build is from revision 16233. I haven't really looked into the code, but removing line 294 seems fix the situation. I was hoping someone would be familiar enough with that code to know if it's supposed to be like that. I'll poke around some more when I have time. Thanks, Dale On Wed, Sep 30, 2009 at 1:19 AM, Shlomy Reinstein <sre...@gm...> wrote: > Without knowing the code, just a brief look shows me this is a > mode-specific issue that is probably related to delegation of mode > attributes. So, in which mode does it happen? Maybe something's wrong > with the mode file? > > Shlomy > > On Wed, Sep 30, 2009 at 9:03 AM, Matthieu Casanova > <cho...@gm...> wrote: >> Hi, I don't know exactly, but I suppose those keywords are used in >> handleRule, don't you think ? >> >> 2009/9/29 Dale Anson <da...@gr...> >>> >>> In TokenMarker.java, there are these lines starting at line 292: >>> >>> 292: LineContext tempContext = context; >>> 293: context = context.parent; >>> 294: keywords = context.rules.getKeywords(); >>> 295: boolean handled = handleRule(rule,true); >>> 296: context = tempContext; >>> 297: keywords = context.rules.getKeywords(); >>> >>> >>> Does anyone know why the line at 294 is there at all? The value for >>> 'keywords' is replaced in line 297 and it isn't used in between. I'm >>> getting null pointer exceptions from line 294 fairly often. Just >>> hitting the "Enter" button while typing in the text area causes it, >>> then there is a beanshell error dialog and the cursor goes to the >>> wrong place, there are painting issues, and so on. I've removed line >>> 294 in my local copy, and things seem to be working better so far. >>> I'm not familiar with this piece of code at all, so I don't know if >>> context.rules on line 294 should ever be null. >>> >>> Thanks, >>> >>> Dale >>> >>> >>> ------------------------------------------------------------------------------ >>> Come build with us! The BlackBerry® Developer Conference in SF, CA >>> is the only developer event you need to attend this year. Jumpstart your >>> developing skills, take BlackBerry mobile applications to market and stay >>> ahead of the curve. Join us from November 9-12, 2009. Register >>> now! >>> http://p.sf.net/sfu/devconf >>> -- >>> ----------------------------------------------- >>> jEdit Developers' List >>> jEd...@li... >>> https://lists.sourceforge.net/lists/listinfo/jedit-devel >> >> >> ------------------------------------------------------------------------------ >> Come build with us! The BlackBerry® Developer Conference in SF, CA >> is the only developer event you need to attend this year. Jumpstart your >> developing skills, take BlackBerry mobile applications to market and stay >> ahead of the curve. Join us from November 9-12, 2009. Register now! >> http://p.sf.net/sfu/devconf >> -- >> ----------------------------------------------- >> jEdit Developers' List >> jEd...@li... >> https://lists.sourceforge.net/lists/listinfo/jedit-devel >> >> > |
|
From: Alan E. <ala...@gm...> - 2009-09-30 15:43:00
|
My guess is that the only person who is really familiar with that code is Slava. On Wed, Sep 30, 2009 at 8:06 AM, Dale Anson <da...@gr...> wrote: > This happens when editing java files. I am using head of svn, my > current build is from revision 16233. I haven't really looked into > the code, but removing line 294 seems fix the situation. I was hoping > someone would be familiar enough with that code to know if it's > supposed to be like that. I'll poke around some more when I have > time. > > Thanks, |
|
From: Matthieu C. <cho...@gm...> - 2009-09-30 15:58:40
|
But in that code, the fields are used as parameters, so filling the keyword field at line 294 is used to highlight tokens, removing that will lead to bugs. For the NPE at line 294, do you know if it is because of context being null or context.rules ? 2009/9/30 Dale Anson <da...@gr...> > This happens when editing java files. I am using head of svn, my > current build is from revision 16233. I haven't really looked into > the code, but removing line 294 seems fix the situation. I was hoping > someone would be familiar enough with that code to know if it's > supposed to be like that. I'll poke around some more when I have > time. > > Thanks, > > Dale > > > On Wed, Sep 30, 2009 at 1:19 AM, Shlomy Reinstein <sre...@gm...> > wrote: > > Without knowing the code, just a brief look shows me this is a > > mode-specific issue that is probably related to delegation of mode > > attributes. So, in which mode does it happen? Maybe something's wrong > > with the mode file? > > > > Shlomy > > > > On Wed, Sep 30, 2009 at 9:03 AM, Matthieu Casanova > > <cho...@gm...> wrote: > >> Hi, I don't know exactly, but I suppose those keywords are used in > >> handleRule, don't you think ? > >> > >> 2009/9/29 Dale Anson <da...@gr...> > >>> > >>> In TokenMarker.java, there are these lines starting at line 292: > >>> > >>> 292: LineContext tempContext = context; > >>> 293: context = context.parent; > >>> 294: keywords = context.rules.getKeywords(); > >>> 295: boolean handled = handleRule(rule,true); > >>> 296: context = tempContext; > >>> 297: keywords = context.rules.getKeywords(); > >>> > >>> > >>> Does anyone know why the line at 294 is there at all? The value for > >>> 'keywords' is replaced in line 297 and it isn't used in between. I'm > >>> getting null pointer exceptions from line 294 fairly often. Just > >>> hitting the "Enter" button while typing in the text area causes it, > >>> then there is a beanshell error dialog and the cursor goes to the > >>> wrong place, there are painting issues, and so on. I've removed line > >>> 294 in my local copy, and things seem to be working better so far. > >>> I'm not familiar with this piece of code at all, so I don't know if > >>> context.rules on line 294 should ever be null. > >>> > >>> Thanks, > >>> > >>> Dale > >>> > >>> > >>> > ------------------------------------------------------------------------------ > >>> Come build with us! The BlackBerry® Developer Conference in SF, CA > >>> is the only developer event you need to attend this year. Jumpstart > your > >>> developing skills, take BlackBerry mobile applications to market and > stay > >>> ahead of the curve. Join us from November 9-12, 2009. Register > >>> now! > >>> http://p.sf.net/sfu/devconf > >>> -- > >>> ----------------------------------------------- > >>> jEdit Developers' List > >>> jEd...@li... > >>> https://lists.sourceforge.net/lists/listinfo/jedit-devel > >> > >> > >> > ------------------------------------------------------------------------------ > >> Come build with us! The BlackBerry® Developer Conference in SF, CA > >> is the only developer event you need to attend this year. Jumpstart your > >> developing skills, take BlackBerry mobile applications to market and > stay > >> ahead of the curve. Join us from November 9-12, 2009. Register > now! > >> http://p.sf.net/sfu/devconf > >> -- > >> ----------------------------------------------- > >> jEdit Developers' List > >> jEd...@li... > >> https://lists.sourceforge.net/lists/listinfo/jedit-devel > >> > >> > > > > > ------------------------------------------------------------------------------ > Come build with us! The BlackBerry® Developer Conference in SF, CA > is the only developer event you need to attend this year. Jumpstart your > developing skills, take BlackBerry mobile applications to market and stay > ahead of the curve. Join us from November 9-12, 2009. Register now! > http://p.sf.net/sfu/devconf > -- > ----------------------------------------------- > jEdit Developers' List > jEd...@li... > https://lists.sourceforge.net/lists/listinfo/jedit-devel > |
|
From: Dale A. <da...@gr...> - 2009-09-30 18:50:26
|
On Wed, Sep 30, 2009 at 9:58 AM, Matthieu Casanova <cho...@gm...> wrote: > But in that code, the fields are used as parameters, so filling the keyword > field at line 294 is used to highlight tokens, removing that will lead to > bugs. > For the NPE at line 294, do you know if it is because of context being null > or context.rules ? I don't know. I didn't look into it far enough. I supposed, though, if I'm the only one seeing this issue, that means I've got something wrong somewhere. > > 2009/9/30 Dale Anson <da...@gr...> >> >> This happens when editing java files. I am using head of svn, my >> current build is from revision 16233. I haven't really looked into >> the code, but removing line 294 seems fix the situation. I was hoping >> someone would be familiar enough with that code to know if it's >> supposed to be like that. I'll poke around some more when I have >> time. >> >> Thanks, >> >> Dale >> >> >> On Wed, Sep 30, 2009 at 1:19 AM, Shlomy Reinstein <sre...@gm...> >> wrote: >> > Without knowing the code, just a brief look shows me this is a >> > mode-specific issue that is probably related to delegation of mode >> > attributes. So, in which mode does it happen? Maybe something's wrong >> > with the mode file? >> > >> > Shlomy >> > >> > On Wed, Sep 30, 2009 at 9:03 AM, Matthieu Casanova >> > <cho...@gm...> wrote: >> >> Hi, I don't know exactly, but I suppose those keywords are used in >> >> handleRule, don't you think ? >> >> >> >> 2009/9/29 Dale Anson <da...@gr...> >> >>> >> >>> In TokenMarker.java, there are these lines starting at line 292: >> >>> >> >>> 292: LineContext tempContext = context; >> >>> 293: context = context.parent; >> >>> 294: keywords = context.rules.getKeywords(); >> >>> 295: boolean handled = handleRule(rule,true); >> >>> 296: context = tempContext; >> >>> 297: keywords = context.rules.getKeywords(); >> >>> >> >>> >> >>> Does anyone know why the line at 294 is there at all? The value for >> >>> 'keywords' is replaced in line 297 and it isn't used in between. I'm >> >>> getting null pointer exceptions from line 294 fairly often. Just >> >>> hitting the "Enter" button while typing in the text area causes it, >> >>> then there is a beanshell error dialog and the cursor goes to the >> >>> wrong place, there are painting issues, and so on. I've removed line >> >>> 294 in my local copy, and things seem to be working better so far. >> >>> I'm not familiar with this piece of code at all, so I don't know if >> >>> context.rules on line 294 should ever be null. >> >>> >> >>> Thanks, >> >>> >> >>> Dale >> >>> >> >>> >> >>> >> >>> ------------------------------------------------------------------------------ >> >>> Come build with us! The BlackBerry® Developer Conference in SF, CA >> >>> is the only developer event you need to attend this year. Jumpstart >> >>> your >> >>> developing skills, take BlackBerry mobile applications to market and >> >>> stay >> >>> ahead of the curve. Join us from November 9-12, 2009. Register >> >>> now! >> >>> http://p.sf.net/sfu/devconf >> >>> -- >> >>> ----------------------------------------------- >> >>> jEdit Developers' List >> >>> jEd...@li... >> >>> https://lists.sourceforge.net/lists/listinfo/jedit-devel >> >> >> >> >> >> >> >> ------------------------------------------------------------------------------ >> >> Come build with us! The BlackBerry® Developer Conference in SF, CA >> >> is the only developer event you need to attend this year. Jumpstart >> >> your >> >> developing skills, take BlackBerry mobile applications to market and >> >> stay >> >> ahead of the curve. Join us from November 9-12, 2009. Register >> >> now! >> >> http://p.sf.net/sfu/devconf >> >> -- >> >> ----------------------------------------------- >> >> jEdit Developers' List >> >> jEd...@li... >> >> https://lists.sourceforge.net/lists/listinfo/jedit-devel >> >> >> >> >> > >> >> >> ------------------------------------------------------------------------------ >> Come build with us! The BlackBerry® Developer Conference in SF, CA >> is the only developer event you need to attend this year. Jumpstart your >> developing skills, take BlackBerry mobile applications to market and stay >> ahead of the curve. Join us from November 9-12, 2009. Register >> now! >> http://p.sf.net/sfu/devconf >> -- >> ----------------------------------------------- >> jEdit Developers' List >> jEd...@li... >> https://lists.sourceforge.net/lists/listinfo/jedit-devel > > |
|
From: Matthieu C. <cho...@gm...> - 2009-09-30 19:02:25
|
Maybe you use an edit mode that cause the problem ? 2009/9/30 Dale Anson <da...@gr...> > On Wed, Sep 30, 2009 at 9:58 AM, Matthieu Casanova > <cho...@gm...> wrote: > > But in that code, the fields are used as parameters, so filling the > keyword > > field at line 294 is used to highlight tokens, removing that will lead to > > bugs. > > For the NPE at line 294, do you know if it is because of context being > null > > or context.rules ? > > I don't know. I didn't look into it far enough. I supposed, though, > if I'm the only one seeing this issue, that means I've got something > wrong somewhere. > > > > > > 2009/9/30 Dale Anson <da...@gr...> > >> > >> This happens when editing java files. I am using head of svn, my > >> current build is from revision 16233. I haven't really looked into > >> the code, but removing line 294 seems fix the situation. I was hoping > >> someone would be familiar enough with that code to know if it's > >> supposed to be like that. I'll poke around some more when I have > >> time. > >> > >> Thanks, > >> > >> Dale > >> > >> > >> On Wed, Sep 30, 2009 at 1:19 AM, Shlomy Reinstein <sre...@gm...> > >> wrote: > >> > Without knowing the code, just a brief look shows me this is a > >> > mode-specific issue that is probably related to delegation of mode > >> > attributes. So, in which mode does it happen? Maybe something's wrong > >> > with the mode file? > >> > > >> > Shlomy > >> > > >> > On Wed, Sep 30, 2009 at 9:03 AM, Matthieu Casanova > >> > <cho...@gm...> wrote: > >> >> Hi, I don't know exactly, but I suppose those keywords are used in > >> >> handleRule, don't you think ? > >> >> > >> >> 2009/9/29 Dale Anson <da...@gr...> > >> >>> > >> >>> In TokenMarker.java, there are these lines starting at line 292: > >> >>> > >> >>> 292: LineContext tempContext = context; > >> >>> 293: context = context.parent; > >> >>> 294: keywords = context.rules.getKeywords(); > >> >>> 295: boolean handled = handleRule(rule,true); > >> >>> 296: context = tempContext; > >> >>> 297: keywords = context.rules.getKeywords(); > >> >>> > >> >>> > >> >>> Does anyone know why the line at 294 is there at all? The value for > >> >>> 'keywords' is replaced in line 297 and it isn't used in between. > I'm > >> >>> getting null pointer exceptions from line 294 fairly often. Just > >> >>> hitting the "Enter" button while typing in the text area causes it, > >> >>> then there is a beanshell error dialog and the cursor goes to the > >> >>> wrong place, there are painting issues, and so on. I've removed > line > >> >>> 294 in my local copy, and things seem to be working better so far. > >> >>> I'm not familiar with this piece of code at all, so I don't know if > >> >>> context.rules on line 294 should ever be null. > >> >>> > >> >>> Thanks, > >> >>> > >> >>> Dale > >> >>> > >> >>> > >> >>> > >> >>> > ------------------------------------------------------------------------------ > >> >>> Come build with us! The BlackBerry® Developer Conference in SF, > CA > >> >>> is the only developer event you need to attend this year. Jumpstart > >> >>> your > >> >>> developing skills, take BlackBerry mobile applications to market and > >> >>> stay > >> >>> ahead of the curve. Join us from November 9-12, 2009. Register > >> >>> now! > >> >>> http://p.sf.net/sfu/devconf > >> >>> -- > >> >>> ----------------------------------------------- > >> >>> jEdit Developers' List > >> >>> jEd...@li... > >> >>> https://lists.sourceforge.net/lists/listinfo/jedit-devel > >> >> > >> >> > >> >> > >> >> > ------------------------------------------------------------------------------ > >> >> Come build with us! The BlackBerry® Developer Conference in SF, > CA > >> >> is the only developer event you need to attend this year. Jumpstart > >> >> your > >> >> developing skills, take BlackBerry mobile applications to market and > >> >> stay > >> >> ahead of the curve. Join us from November 9-12, 2009. Register > >> >> now! > >> >> http://p.sf.net/sfu/devconf > >> >> -- > >> >> ----------------------------------------------- > >> >> jEdit Developers' List > >> >> jEd...@li... > >> >> https://lists.sourceforge.net/lists/listinfo/jedit-devel > >> >> > >> >> > >> > > >> > >> > >> > ------------------------------------------------------------------------------ > >> Come build with us! The BlackBerry® Developer Conference in SF, CA > >> is the only developer event you need to attend this year. Jumpstart your > >> developing skills, take BlackBerry mobile applications to market and > stay > >> ahead of the curve. Join us from November 9-12, 2009. Register > >> now! > >> http://p.sf.net/sfu/devconf > >> -- > >> ----------------------------------------------- > >> jEdit Developers' List > >> jEd...@li... > >> https://lists.sourceforge.net/lists/listinfo/jedit-devel > > > > > |
|
From: Alan E. <ala...@gm...> - 2009-09-30 19:08:04
|
I've seen an NPE there from time to time also, but I can't reproduce it. On Wed, Sep 30, 2009 at 11:50 AM, Dale Anson <da...@gr...> wrote: > On Wed, Sep 30, 2009 at 9:58 AM, Matthieu Casanova > <cho...@gm...> wrote: > > But in that code, the fields are used as parameters, so filling the > keyword > > field at line 294 is used to highlight tokens, removing that will lead to > > bugs. > > For the NPE at line 294, do you know if it is because of context being > null > > or context.rules ? > > I don't know. I didn't look into it far enough. I supposed, though, > if I'm the only one seeing this issue, that means I've got something > wrong somewhere. > > |
|
From: Dale A. <da...@gr...> - 2009-10-02 02:26:56
|
I entered a patch to fix this, see tracker 2871624. I did some more investigation, and it turns out that context.parent can be null. The patch checks for this case, and if the parent is null, it just continues to use the same context. So far, this seems to be working fine. Dale On Wed, Sep 30, 2009 at 1:07 PM, Alan Ezust <ala...@gm...> wrote: > I've seen an NPE there from time to time also, but I can't reproduce it. > > On Wed, Sep 30, 2009 at 11:50 AM, Dale Anson <da...@gr...> wrote: >> >> On Wed, Sep 30, 2009 at 9:58 AM, Matthieu Casanova >> <cho...@gm...> wrote: >> > But in that code, the fields are used as parameters, so filling the >> > keyword >> > field at line 294 is used to highlight tokens, removing that will lead >> > to >> > bugs. >> > For the NPE at line 294, do you know if it is because of context being >> > null >> > or context.rules ? >> >> I don't know. I didn't look into it far enough. I supposed, though, >> if I'm the only one seeing this issue, that means I've got something >> wrong somewhere. >> > > |
|
From: Shlomy R. <sre...@gm...> - 2009-10-03 09:40:26
|
Hi,
It turns out that TokenMarker.markTokens is not thread-safe. It needs
to be synchronized. It has a 'context' field, that is potentially
updated each time "markTokens" is called. If called simultaneously by
multiple threads, each may modify it, causing the other threads to use
an incorrect context.
I just verified that the problem happens very frequently with the
TaskList plugin. TaskList has a dockable which shows the tasks in the
current buffer, and also highlights tasks in the text area. To find
the tasks in the buffer, it uses "markTokens" in the background. So,
when this plugin is in use, I experience all kinds of painting issues
and occasionally the NPE that Dale experienced.
Disabling the TaskList plugin gets rid of all these problems.
However, it is not a TaskList problem, "markTokens" simply needs to be
synchronized. If this causes some performance problem in the core,
then we should have two separate paths to markTokens, one for the core
and one for plugins, and make the plugin's path ensure thread-safety,
e.g. by deferring the task to the AWT thread.
Shlomy
On Sat, Oct 3, 2009 at 3:39 AM, Dale Anson <da...@gr...> wrote:
> Actually, I verified that context.parent is occasionally null, which
> causes the NPE when its 'rules' field is referenced. Thinking about
> this some more, I bet is it's a threading issue. It could be that in
> the first check (in the code you sent) it is not null, but is reset to
> null before the method is actually invoked. That would mean my patch
> is not very good since it doesn't address thread-safety.
>
> I did double check, I've got source for jEdit at revision 16233. The
> only change I have is the one I made to this class. I verified that
> I'm using the standard java mode from svn also, and it's while editing
> java files that I know I've seen the problem. I'm not sure if I've
> seen this issue in other modes.
>
> Dale
>
>
> On Fri, Oct 2, 2009 at 2:46 AM, Shlomy Reinstein <sre...@gm...> wrote:
>> I took a more thorough look at the code. It is absolutely IMPOSSIBLE
>> that context.parent is null in the method that generates the NPE for
>> you, unless you've changed something in the jedit core. There is only
>> one place where the method generating the NPE is called:
>> //{{{ check for end of delegate
>> if (context.parent != null
>> && context.parent.inRule != null
>> && checkDelegateEnd(context.parent.inRule))
>> {
>> seenWhitespaceEnd = true;
>> continue main_loop;
>> } //}}}
>>
>> Note the check for "context.parent" not being null before calling this method.
>> An NPE can still be caused by this line, but it won't be for
>> "context.parent" being null, rather it would be for
>> "context.parent.rules" being null.
>>
>> Shlomy
>>
>> On Fri, Oct 2, 2009 at 8:15 AM, Dale Anson <da...@gr...> wrote:
>>> No, I can't reproduce it reliably. Somehow, I get into this state
>>> after a couple of hours of running jEdit. Usually, it happens when
>>> I'm editing somewhere in the middle of a file, I'll hit Enter at the
>>> end of a line to start the next line and then there is the trouble. I
>>> have verified that it is definitely the case that context.parent is
>>> sometimes null. I see in LineContext itself that there are a several
>>> checks for a null parent, but not elsewhere. I think I've only seen
>>> this with java files, but I'm not absolutely certain.
>>>
>>> Do you know of a better thing to do if context.parent is null?
>>>
>>> Dale
>>>
>>>
>>> On Thu, Oct 1, 2009 at 11:31 PM, Shlomy Reinstein <sre...@gm...> wrote:
>>>> Hi,
>>>> Can you reproduce this every time with some example?
>>>> If not , does this happen when you are editing (i.e. pressing enter
>>>> on) the first or second line, or also when editing subsequent lines
>>>> down the file?
>>>> The 'parent' is usually the LineContext of the previous line.
>>>> Shlomy
>>>>
>>>> On Fri, Oct 2, 2009 at 3:33 AM, Dale Anson <da...@gr...> wrote:
>>>>> I entered a patch to fix this, see tracker 2871624. I did some more
>>>>> investigation, and it turns out that context.parent can be null. The
>>>>> patch checks for this case, and if the parent is null, it just
>>>>> continues to use the same context. So far, this seems to be working
>>>>> fine.
>>>>>
>>>>> Dale
>>>>>
>>>>>
>>>>> On Wed, Sep 30, 2009 at 1:07 PM, Alan Ezust <ala...@gm...> wrote:
>>>>>> I've seen an NPE there from time to time also, but I can't reproduce it.
>>>>>>
>>>>>> On Wed, Sep 30, 2009 at 11:50 AM, Dale Anson <da...@gr...> wrote:
>>>>>>>
>>>>>>> On Wed, Sep 30, 2009 at 9:58 AM, Matthieu Casanova
>>>>>>> <cho...@gm...> wrote:
>>>>>>> > But in that code, the fields are used as parameters, so filling the
>>>>>>> > keyword
>>>>>>> > field at line 294 is used to highlight tokens, removing that will lead
>>>>>>> > to
>>>>>>> > bugs.
>>>>>>> > For the NPE at line 294, do you know if it is because of context being
>>>>>>> > null
>>>>>>> > or context.rules ?
>>>>>>>
>>>>>>> I don't know. I didn't look into it far enough. I supposed, though,
>>>>>>> if I'm the only one seeing this issue, that means I've got something
>>>>>>> wrong somewhere.
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>> Come build with us! The BlackBerry® Developer Conference in SF, CA
>>>>> is the only developer event you need to attend this year. Jumpstart your
>>>>> developing skills, take BlackBerry mobile applications to market and stay
>>>>> ahead of the curve. Join us from November 9-12, 2009. Register now!
>>>>> http://p.sf.net/sfu/devconf
>>>>> --
>>>>> -----------------------------------------------
>>>>> jEdit Developers' List
>>>>> jEd...@li...
>>>>> https://lists.sourceforge.net/lists/listinfo/jedit-devel
>>>>>
>>>>
>>>
>>
>
|
|
From: Shlomy R. <sre...@gm...> - 2009-10-03 13:39:19
|
I have committed this small change (make TokenMarker.markTokens
synchronized) and it seems to solve the issue with TaskList.
I also haven't noticed any performance impact of this. If anyone
discovers a performance problem caused by this, let me know and I'll
fix it.
Shlomy
On Sat, Oct 3, 2009 at 11:40 AM, Shlomy Reinstein <sre...@gm...> wrote:
> Hi,
>
> It turns out that TokenMarker.markTokens is not thread-safe. It needs
> to be synchronized. It has a 'context' field, that is potentially
> updated each time "markTokens" is called. If called simultaneously by
> multiple threads, each may modify it, causing the other threads to use
> an incorrect context.
>
> I just verified that the problem happens very frequently with the
> TaskList plugin. TaskList has a dockable which shows the tasks in the
> current buffer, and also highlights tasks in the text area. To find
> the tasks in the buffer, it uses "markTokens" in the background. So,
> when this plugin is in use, I experience all kinds of painting issues
> and occasionally the NPE that Dale experienced.
> Disabling the TaskList plugin gets rid of all these problems.
>
> However, it is not a TaskList problem, "markTokens" simply needs to be
> synchronized. If this causes some performance problem in the core,
> then we should have two separate paths to markTokens, one for the core
> and one for plugins, and make the plugin's path ensure thread-safety,
> e.g. by deferring the task to the AWT thread.
>
> Shlomy
>
> On Sat, Oct 3, 2009 at 3:39 AM, Dale Anson <da...@gr...> wrote:
>> Actually, I verified that context.parent is occasionally null, which
>> causes the NPE when its 'rules' field is referenced. Thinking about
>> this some more, I bet is it's a threading issue. It could be that in
>> the first check (in the code you sent) it is not null, but is reset to
>> null before the method is actually invoked. That would mean my patch
>> is not very good since it doesn't address thread-safety.
>>
>> I did double check, I've got source for jEdit at revision 16233. The
>> only change I have is the one I made to this class. I verified that
>> I'm using the standard java mode from svn also, and it's while editing
>> java files that I know I've seen the problem. I'm not sure if I've
>> seen this issue in other modes.
>>
>> Dale
>>
>>
>> On Fri, Oct 2, 2009 at 2:46 AM, Shlomy Reinstein <sre...@gm...> wrote:
>>> I took a more thorough look at the code. It is absolutely IMPOSSIBLE
>>> that context.parent is null in the method that generates the NPE for
>>> you, unless you've changed something in the jedit core. There is only
>>> one place where the method generating the NPE is called:
>>> //{{{ check for end of delegate
>>> if (context.parent != null
>>> && context.parent.inRule != null
>>> && checkDelegateEnd(context.parent.inRule))
>>> {
>>> seenWhitespaceEnd = true;
>>> continue main_loop;
>>> } //}}}
>>>
>>> Note the check for "context.parent" not being null before calling this method.
>>> An NPE can still be caused by this line, but it won't be for
>>> "context.parent" being null, rather it would be for
>>> "context.parent.rules" being null.
>>>
>>> Shlomy
>>>
>>> On Fri, Oct 2, 2009 at 8:15 AM, Dale Anson <da...@gr...> wrote:
>>>> No, I can't reproduce it reliably. Somehow, I get into this state
>>>> after a couple of hours of running jEdit. Usually, it happens when
>>>> I'm editing somewhere in the middle of a file, I'll hit Enter at the
>>>> end of a line to start the next line and then there is the trouble. I
>>>> have verified that it is definitely the case that context.parent is
>>>> sometimes null. I see in LineContext itself that there are a several
>>>> checks for a null parent, but not elsewhere. I think I've only seen
>>>> this with java files, but I'm not absolutely certain.
>>>>
>>>> Do you know of a better thing to do if context.parent is null?
>>>>
>>>> Dale
>>>>
>>>>
>>>> On Thu, Oct 1, 2009 at 11:31 PM, Shlomy Reinstein <sre...@gm...> wrote:
>>>>> Hi,
>>>>> Can you reproduce this every time with some example?
>>>>> If not , does this happen when you are editing (i.e. pressing enter
>>>>> on) the first or second line, or also when editing subsequent lines
>>>>> down the file?
>>>>> The 'parent' is usually the LineContext of the previous line.
>>>>> Shlomy
>>>>>
>>>>> On Fri, Oct 2, 2009 at 3:33 AM, Dale Anson <da...@gr...> wrote:
>>>>>> I entered a patch to fix this, see tracker 2871624. I did some more
>>>>>> investigation, and it turns out that context.parent can be null. The
>>>>>> patch checks for this case, and if the parent is null, it just
>>>>>> continues to use the same context. So far, this seems to be working
>>>>>> fine.
>>>>>>
>>>>>> Dale
>>>>>>
>>>>>>
>>>>>> On Wed, Sep 30, 2009 at 1:07 PM, Alan Ezust <ala...@gm...> wrote:
>>>>>>> I've seen an NPE there from time to time also, but I can't reproduce it.
>>>>>>>
>>>>>>> On Wed, Sep 30, 2009 at 11:50 AM, Dale Anson <da...@gr...> wrote:
>>>>>>>>
>>>>>>>> On Wed, Sep 30, 2009 at 9:58 AM, Matthieu Casanova
>>>>>>>> <cho...@gm...> wrote:
>>>>>>>> > But in that code, the fields are used as parameters, so filling the
>>>>>>>> > keyword
>>>>>>>> > field at line 294 is used to highlight tokens, removing that will lead
>>>>>>>> > to
>>>>>>>> > bugs.
>>>>>>>> > For the NPE at line 294, do you know if it is because of context being
>>>>>>>> > null
>>>>>>>> > or context.rules ?
>>>>>>>>
>>>>>>>> I don't know. I didn't look into it far enough. I supposed, though,
>>>>>>>> if I'm the only one seeing this issue, that means I've got something
>>>>>>>> wrong somewhere.
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> ------------------------------------------------------------------------------
>>>>>> Come build with us! The BlackBerry® Developer Conference in SF, CA
>>>>>> is the only developer event you need to attend this year. Jumpstart your
>>>>>> developing skills, take BlackBerry mobile applications to market and stay
>>>>>> ahead of the curve. Join us from November 9-12, 2009. Register now!
>>>>>> http://p.sf.net/sfu/devconf
>>>>>> --
>>>>>> -----------------------------------------------
>>>>>> jEdit Developers' List
>>>>>> jEd...@li...
>>>>>> https://lists.sourceforge.net/lists/listinfo/jedit-devel
>>>>>>
>>>>>
>>>>
>>>
>>
>
|
|
From: Dale A. <da...@gr...> - 2009-10-03 15:09:06
|
What was the issue with TaskList? And which TaskList -- the one in
PluginManager or the one from Eric's daily builds?
Dale
On Sat, Oct 3, 2009 at 7:39 AM, Shlomy Reinstein <sre...@gm...> wrote:
> I have committed this small change (make TokenMarker.markTokens
> synchronized) and it seems to solve the issue with TaskList.
> I also haven't noticed any performance impact of this. If anyone
> discovers a performance problem caused by this, let me know and I'll
> fix it.
>
> Shlomy
>
> On Sat, Oct 3, 2009 at 11:40 AM, Shlomy Reinstein <sre...@gm...> wrote:
>> Hi,
>>
>> It turns out that TokenMarker.markTokens is not thread-safe. It needs
>> to be synchronized. It has a 'context' field, that is potentially
>> updated each time "markTokens" is called. If called simultaneously by
>> multiple threads, each may modify it, causing the other threads to use
>> an incorrect context.
>>
>> I just verified that the problem happens very frequently with the
>> TaskList plugin. TaskList has a dockable which shows the tasks in the
>> current buffer, and also highlights tasks in the text area. To find
>> the tasks in the buffer, it uses "markTokens" in the background. So,
>> when this plugin is in use, I experience all kinds of painting issues
>> and occasionally the NPE that Dale experienced.
>> Disabling the TaskList plugin gets rid of all these problems.
>>
>> However, it is not a TaskList problem, "markTokens" simply needs to be
>> synchronized. If this causes some performance problem in the core,
>> then we should have two separate paths to markTokens, one for the core
>> and one for plugins, and make the plugin's path ensure thread-safety,
>> e.g. by deferring the task to the AWT thread.
>>
>> Shlomy
>>
>> On Sat, Oct 3, 2009 at 3:39 AM, Dale Anson <da...@gr...> wrote:
>>> Actually, I verified that context.parent is occasionally null, which
>>> causes the NPE when its 'rules' field is referenced. Thinking about
>>> this some more, I bet is it's a threading issue. It could be that in
>>> the first check (in the code you sent) it is not null, but is reset to
>>> null before the method is actually invoked. That would mean my patch
>>> is not very good since it doesn't address thread-safety.
>>>
>>> I did double check, I've got source for jEdit at revision 16233. The
>>> only change I have is the one I made to this class. I verified that
>>> I'm using the standard java mode from svn also, and it's while editing
>>> java files that I know I've seen the problem. I'm not sure if I've
>>> seen this issue in other modes.
>>>
>>> Dale
>>>
>>>
>>> On Fri, Oct 2, 2009 at 2:46 AM, Shlomy Reinstein <sre...@gm...> wrote:
>>>> I took a more thorough look at the code. It is absolutely IMPOSSIBLE
>>>> that context.parent is null in the method that generates the NPE for
>>>> you, unless you've changed something in the jedit core. There is only
>>>> one place where the method generating the NPE is called:
>>>> //{{{ check for end of delegate
>>>> if (context.parent != null
>>>> && context.parent.inRule != null
>>>> && checkDelegateEnd(context.parent.inRule))
>>>> {
>>>> seenWhitespaceEnd = true;
>>>> continue main_loop;
>>>> } //}}}
>>>>
>>>> Note the check for "context.parent" not being null before calling this method.
>>>> An NPE can still be caused by this line, but it won't be for
>>>> "context.parent" being null, rather it would be for
>>>> "context.parent.rules" being null.
>>>>
>>>> Shlomy
>>>>
>>>> On Fri, Oct 2, 2009 at 8:15 AM, Dale Anson <da...@gr...> wrote:
>>>>> No, I can't reproduce it reliably. Somehow, I get into this state
>>>>> after a couple of hours of running jEdit. Usually, it happens when
>>>>> I'm editing somewhere in the middle of a file, I'll hit Enter at the
>>>>> end of a line to start the next line and then there is the trouble. I
>>>>> have verified that it is definitely the case that context.parent is
>>>>> sometimes null. I see in LineContext itself that there are a several
>>>>> checks for a null parent, but not elsewhere. I think I've only seen
>>>>> this with java files, but I'm not absolutely certain.
>>>>>
>>>>> Do you know of a better thing to do if context.parent is null?
>>>>>
>>>>> Dale
>>>>>
>>>>>
>>>>> On Thu, Oct 1, 2009 at 11:31 PM, Shlomy Reinstein <sre...@gm...> wrote:
>>>>>> Hi,
>>>>>> Can you reproduce this every time with some example?
>>>>>> If not , does this happen when you are editing (i.e. pressing enter
>>>>>> on) the first or second line, or also when editing subsequent lines
>>>>>> down the file?
>>>>>> The 'parent' is usually the LineContext of the previous line.
>>>>>> Shlomy
>>>>>>
>>>>>> On Fri, Oct 2, 2009 at 3:33 AM, Dale Anson <da...@gr...> wrote:
>>>>>>> I entered a patch to fix this, see tracker 2871624. I did some more
>>>>>>> investigation, and it turns out that context.parent can be null. The
>>>>>>> patch checks for this case, and if the parent is null, it just
>>>>>>> continues to use the same context. So far, this seems to be working
>>>>>>> fine.
>>>>>>>
>>>>>>> Dale
>>>>>>>
>>>>>>>
>>>>>>> On Wed, Sep 30, 2009 at 1:07 PM, Alan Ezust <ala...@gm...> wrote:
>>>>>>>> I've seen an NPE there from time to time also, but I can't reproduce it.
>>>>>>>>
>>>>>>>> On Wed, Sep 30, 2009 at 11:50 AM, Dale Anson <da...@gr...> wrote:
>>>>>>>>>
>>>>>>>>> On Wed, Sep 30, 2009 at 9:58 AM, Matthieu Casanova
>>>>>>>>> <cho...@gm...> wrote:
>>>>>>>>> > But in that code, the fields are used as parameters, so filling the
>>>>>>>>> > keyword
>>>>>>>>> > field at line 294 is used to highlight tokens, removing that will lead
>>>>>>>>> > to
>>>>>>>>> > bugs.
>>>>>>>>> > For the NPE at line 294, do you know if it is because of context being
>>>>>>>>> > null
>>>>>>>>> > or context.rules ?
>>>>>>>>>
>>>>>>>>> I don't know. I didn't look into it far enough. I supposed, though,
>>>>>>>>> if I'm the only one seeing this issue, that means I've got something
>>>>>>>>> wrong somewhere.
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> ------------------------------------------------------------------------------
>>>>>>> Come build with us! The BlackBerry® Developer Conference in SF, CA
>>>>>>> is the only developer event you need to attend this year. Jumpstart your
>>>>>>> developing skills, take BlackBerry mobile applications to market and stay
>>>>>>> ahead of the curve. Join us from November 9-12, 2009. Register now!
>>>>>>> http://p.sf.net/sfu/devconf
>>>>>>> --
>>>>>>> -----------------------------------------------
>>>>>>> jEdit Developers' List
>>>>>>> jEd...@li...
>>>>>>> https://lists.sourceforge.net/lists/listinfo/jedit-devel
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
|
|
From: Shlomy R. <sre...@gm...> - 2009-10-03 16:16:40
|
The issue was not in TaskList. The issue was with markTokens not being
thread-safe, being called from the core and from TaskList at the same
time, each call setting the context field to whatever value is right
for it, affecting the other.
I think it was a Plugin Manager's download, nevertheless, I don't
think it matters because TaskList always uses markTokens, doesn't it?
Shlomy
On Sat, Oct 3, 2009 at 5:08 PM, Dale Anson <da...@gr...> wrote:
> What was the issue with TaskList? And which TaskList -- the one in
> PluginManager or the one from Eric's daily builds?
>
> Dale
>
>
> On Sat, Oct 3, 2009 at 7:39 AM, Shlomy Reinstein <sre...@gm...> wrote:
>> I have committed this small change (make TokenMarker.markTokens
>> synchronized) and it seems to solve the issue with TaskList.
>> I also haven't noticed any performance impact of this. If anyone
>> discovers a performance problem caused by this, let me know and I'll
>> fix it.
>>
>> Shlomy
>>
>> On Sat, Oct 3, 2009 at 11:40 AM, Shlomy Reinstein <sre...@gm...> wrote:
>>> Hi,
>>>
>>> It turns out that TokenMarker.markTokens is not thread-safe. It needs
>>> to be synchronized. It has a 'context' field, that is potentially
>>> updated each time "markTokens" is called. If called simultaneously by
>>> multiple threads, each may modify it, causing the other threads to use
>>> an incorrect context.
>>>
>>> I just verified that the problem happens very frequently with the
>>> TaskList plugin. TaskList has a dockable which shows the tasks in the
>>> current buffer, and also highlights tasks in the text area. To find
>>> the tasks in the buffer, it uses "markTokens" in the background. So,
>>> when this plugin is in use, I experience all kinds of painting issues
>>> and occasionally the NPE that Dale experienced.
>>> Disabling the TaskList plugin gets rid of all these problems.
>>>
>>> However, it is not a TaskList problem, "markTokens" simply needs to be
>>> synchronized. If this causes some performance problem in the core,
>>> then we should have two separate paths to markTokens, one for the core
>>> and one for plugins, and make the plugin's path ensure thread-safety,
>>> e.g. by deferring the task to the AWT thread.
>>>
>>> Shlomy
>>>
>>> On Sat, Oct 3, 2009 at 3:39 AM, Dale Anson <da...@gr...> wrote:
>>>> Actually, I verified that context.parent is occasionally null, which
>>>> causes the NPE when its 'rules' field is referenced. Thinking about
>>>> this some more, I bet is it's a threading issue. It could be that in
>>>> the first check (in the code you sent) it is not null, but is reset to
>>>> null before the method is actually invoked. That would mean my patch
>>>> is not very good since it doesn't address thread-safety.
>>>>
>>>> I did double check, I've got source for jEdit at revision 16233. The
>>>> only change I have is the one I made to this class. I verified that
>>>> I'm using the standard java mode from svn also, and it's while editing
>>>> java files that I know I've seen the problem. I'm not sure if I've
>>>> seen this issue in other modes.
>>>>
>>>> Dale
>>>>
>>>>
>>>> On Fri, Oct 2, 2009 at 2:46 AM, Shlomy Reinstein <sre...@gm...> wrote:
>>>>> I took a more thorough look at the code. It is absolutely IMPOSSIBLE
>>>>> that context.parent is null in the method that generates the NPE for
>>>>> you, unless you've changed something in the jedit core. There is only
>>>>> one place where the method generating the NPE is called:
>>>>> //{{{ check for end of delegate
>>>>> if (context.parent != null
>>>>> && context.parent.inRule != null
>>>>> && checkDelegateEnd(context.parent.inRule))
>>>>> {
>>>>> seenWhitespaceEnd = true;
>>>>> continue main_loop;
>>>>> } //}}}
>>>>>
>>>>> Note the check for "context.parent" not being null before calling this method.
>>>>> An NPE can still be caused by this line, but it won't be for
>>>>> "context.parent" being null, rather it would be for
>>>>> "context.parent.rules" being null.
>>>>>
>>>>> Shlomy
>>>>>
>>>>> On Fri, Oct 2, 2009 at 8:15 AM, Dale Anson <da...@gr...> wrote:
>>>>>> No, I can't reproduce it reliably. Somehow, I get into this state
>>>>>> after a couple of hours of running jEdit. Usually, it happens when
>>>>>> I'm editing somewhere in the middle of a file, I'll hit Enter at the
>>>>>> end of a line to start the next line and then there is the trouble. I
>>>>>> have verified that it is definitely the case that context.parent is
>>>>>> sometimes null. I see in LineContext itself that there are a several
>>>>>> checks for a null parent, but not elsewhere. I think I've only seen
>>>>>> this with java files, but I'm not absolutely certain.
>>>>>>
>>>>>> Do you know of a better thing to do if context.parent is null?
>>>>>>
>>>>>> Dale
>>>>>>
>>>>>>
>>>>>> On Thu, Oct 1, 2009 at 11:31 PM, Shlomy Reinstein <sre...@gm...> wrote:
>>>>>>> Hi,
>>>>>>> Can you reproduce this every time with some example?
>>>>>>> If not , does this happen when you are editing (i.e. pressing enter
>>>>>>> on) the first or second line, or also when editing subsequent lines
>>>>>>> down the file?
>>>>>>> The 'parent' is usually the LineContext of the previous line.
>>>>>>> Shlomy
>>>>>>>
>>>>>>> On Fri, Oct 2, 2009 at 3:33 AM, Dale Anson <da...@gr...> wrote:
>>>>>>>> I entered a patch to fix this, see tracker 2871624. I did some more
>>>>>>>> investigation, and it turns out that context.parent can be null. The
>>>>>>>> patch checks for this case, and if the parent is null, it just
>>>>>>>> continues to use the same context. So far, this seems to be working
>>>>>>>> fine.
>>>>>>>>
>>>>>>>> Dale
>>>>>>>>
>>>>>>>>
>>>>>>>> On Wed, Sep 30, 2009 at 1:07 PM, Alan Ezust <ala...@gm...> wrote:
>>>>>>>>> I've seen an NPE there from time to time also, but I can't reproduce it.
>>>>>>>>>
>>>>>>>>> On Wed, Sep 30, 2009 at 11:50 AM, Dale Anson <da...@gr...> wrote:
>>>>>>>>>>
>>>>>>>>>> On Wed, Sep 30, 2009 at 9:58 AM, Matthieu Casanova
>>>>>>>>>> <cho...@gm...> wrote:
>>>>>>>>>> > But in that code, the fields are used as parameters, so filling the
>>>>>>>>>> > keyword
>>>>>>>>>> > field at line 294 is used to highlight tokens, removing that will lead
>>>>>>>>>> > to
>>>>>>>>>> > bugs.
>>>>>>>>>> > For the NPE at line 294, do you know if it is because of context being
>>>>>>>>>> > null
>>>>>>>>>> > or context.rules ?
>>>>>>>>>>
>>>>>>>>>> I don't know. I didn't look into it far enough. I supposed, though,
>>>>>>>>>> if I'm the only one seeing this issue, that means I've got something
>>>>>>>>>> wrong somewhere.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>> ------------------------------------------------------------------------------
>>>>>>>> Come build with us! The BlackBerry® Developer Conference in SF, CA
>>>>>>>> is the only developer event you need to attend this year. Jumpstart your
>>>>>>>> developing skills, take BlackBerry mobile applications to market and stay
>>>>>>>> ahead of the curve. Join us from November 9-12, 2009. Register now!
>>>>>>>> http://p.sf.net/sfu/devconf
>>>>>>>> --
>>>>>>>> -----------------------------------------------
>>>>>>>> jEdit Developers' List
>>>>>>>> jEd...@li...
>>>>>>>> https://lists.sourceforge.net/lists/listinfo/jedit-devel
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
> ------------------------------------------------------------------------------
> Come build with us! The BlackBerry® Developer Conference in SF, CA
> is the only developer event you need to attend this year. Jumpstart your
> developing skills, take BlackBerry mobile applications to market and stay
> ahead of the curve. Join us from November 9-12, 2009. Register now!
> http://p.sf.net/sfu/devconf
> --
> -----------------------------------------------
> jEdit Developers' List
> jEd...@li...
> https://lists.sourceforge.net/lists/listinfo/jedit-devel
>
|
|
From: Dale A. <da...@gr...> - 2009-10-03 15:13:36
|
woops, just saw your other email about TaskList.
Wouldn't it be better to make TokenMarker reentrant rather than synchronized?
Dale
On Sat, Oct 3, 2009 at 9:08 AM, Dale Anson <da...@gr...> wrote:
> What was the issue with TaskList? And which TaskList -- the one in
> PluginManager or the one from Eric's daily builds?
>
> Dale
>
>
> On Sat, Oct 3, 2009 at 7:39 AM, Shlomy Reinstein <sre...@gm...> wrote:
>> I have committed this small change (make TokenMarker.markTokens
>> synchronized) and it seems to solve the issue with TaskList.
>> I also haven't noticed any performance impact of this. If anyone
>> discovers a performance problem caused by this, let me know and I'll
>> fix it.
>>
>> Shlomy
>>
>> On Sat, Oct 3, 2009 at 11:40 AM, Shlomy Reinstein <sre...@gm...> wrote:
>>> Hi,
>>>
>>> It turns out that TokenMarker.markTokens is not thread-safe. It needs
>>> to be synchronized. It has a 'context' field, that is potentially
>>> updated each time "markTokens" is called. If called simultaneously by
>>> multiple threads, each may modify it, causing the other threads to use
>>> an incorrect context.
>>>
>>> I just verified that the problem happens very frequently with the
>>> TaskList plugin. TaskList has a dockable which shows the tasks in the
>>> current buffer, and also highlights tasks in the text area. To find
>>> the tasks in the buffer, it uses "markTokens" in the background. So,
>>> when this plugin is in use, I experience all kinds of painting issues
>>> and occasionally the NPE that Dale experienced.
>>> Disabling the TaskList plugin gets rid of all these problems.
>>>
>>> However, it is not a TaskList problem, "markTokens" simply needs to be
>>> synchronized. If this causes some performance problem in the core,
>>> then we should have two separate paths to markTokens, one for the core
>>> and one for plugins, and make the plugin's path ensure thread-safety,
>>> e.g. by deferring the task to the AWT thread.
>>>
>>> Shlomy
>>>
>>> On Sat, Oct 3, 2009 at 3:39 AM, Dale Anson <da...@gr...> wrote:
>>>> Actually, I verified that context.parent is occasionally null, which
>>>> causes the NPE when its 'rules' field is referenced. Thinking about
>>>> this some more, I bet is it's a threading issue. It could be that in
>>>> the first check (in the code you sent) it is not null, but is reset to
>>>> null before the method is actually invoked. That would mean my patch
>>>> is not very good since it doesn't address thread-safety.
>>>>
>>>> I did double check, I've got source for jEdit at revision 16233. The
>>>> only change I have is the one I made to this class. I verified that
>>>> I'm using the standard java mode from svn also, and it's while editing
>>>> java files that I know I've seen the problem. I'm not sure if I've
>>>> seen this issue in other modes.
>>>>
>>>> Dale
>>>>
>>>>
>>>> On Fri, Oct 2, 2009 at 2:46 AM, Shlomy Reinstein <sre...@gm...> wrote:
>>>>> I took a more thorough look at the code. It is absolutely IMPOSSIBLE
>>>>> that context.parent is null in the method that generates the NPE for
>>>>> you, unless you've changed something in the jedit core. There is only
>>>>> one place where the method generating the NPE is called:
>>>>> //{{{ check for end of delegate
>>>>> if (context.parent != null
>>>>> && context.parent.inRule != null
>>>>> && checkDelegateEnd(context.parent.inRule))
>>>>> {
>>>>> seenWhitespaceEnd = true;
>>>>> continue main_loop;
>>>>> } //}}}
>>>>>
>>>>> Note the check for "context.parent" not being null before calling this method.
>>>>> An NPE can still be caused by this line, but it won't be for
>>>>> "context.parent" being null, rather it would be for
>>>>> "context.parent.rules" being null.
>>>>>
>>>>> Shlomy
>>>>>
>>>>> On Fri, Oct 2, 2009 at 8:15 AM, Dale Anson <da...@gr...> wrote:
>>>>>> No, I can't reproduce it reliably. Somehow, I get into this state
>>>>>> after a couple of hours of running jEdit. Usually, it happens when
>>>>>> I'm editing somewhere in the middle of a file, I'll hit Enter at the
>>>>>> end of a line to start the next line and then there is the trouble. I
>>>>>> have verified that it is definitely the case that context.parent is
>>>>>> sometimes null. I see in LineContext itself that there are a several
>>>>>> checks for a null parent, but not elsewhere. I think I've only seen
>>>>>> this with java files, but I'm not absolutely certain.
>>>>>>
>>>>>> Do you know of a better thing to do if context.parent is null?
>>>>>>
>>>>>> Dale
>>>>>>
>>>>>>
>>>>>> On Thu, Oct 1, 2009 at 11:31 PM, Shlomy Reinstein <sre...@gm...> wrote:
>>>>>>> Hi,
>>>>>>> Can you reproduce this every time with some example?
>>>>>>> If not , does this happen when you are editing (i.e. pressing enter
>>>>>>> on) the first or second line, or also when editing subsequent lines
>>>>>>> down the file?
>>>>>>> The 'parent' is usually the LineContext of the previous line.
>>>>>>> Shlomy
>>>>>>>
>>>>>>> On Fri, Oct 2, 2009 at 3:33 AM, Dale Anson <da...@gr...> wrote:
>>>>>>>> I entered a patch to fix this, see tracker 2871624. I did some more
>>>>>>>> investigation, and it turns out that context.parent can be null. The
>>>>>>>> patch checks for this case, and if the parent is null, it just
>>>>>>>> continues to use the same context. So far, this seems to be working
>>>>>>>> fine.
>>>>>>>>
>>>>>>>> Dale
>>>>>>>>
>>>>>>>>
>>>>>>>> On Wed, Sep 30, 2009 at 1:07 PM, Alan Ezust <ala...@gm...> wrote:
>>>>>>>>> I've seen an NPE there from time to time also, but I can't reproduce it.
>>>>>>>>>
>>>>>>>>> On Wed, Sep 30, 2009 at 11:50 AM, Dale Anson <da...@gr...> wrote:
>>>>>>>>>>
>>>>>>>>>> On Wed, Sep 30, 2009 at 9:58 AM, Matthieu Casanova
>>>>>>>>>> <cho...@gm...> wrote:
>>>>>>>>>> > But in that code, the fields are used as parameters, so filling the
>>>>>>>>>> > keyword
>>>>>>>>>> > field at line 294 is used to highlight tokens, removing that will lead
>>>>>>>>>> > to
>>>>>>>>>> > bugs.
>>>>>>>>>> > For the NPE at line 294, do you know if it is because of context being
>>>>>>>>>> > null
>>>>>>>>>> > or context.rules ?
>>>>>>>>>>
>>>>>>>>>> I don't know. I didn't look into it far enough. I supposed, though,
>>>>>>>>>> if I'm the only one seeing this issue, that means I've got something
>>>>>>>>>> wrong somewhere.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>> ------------------------------------------------------------------------------
>>>>>>>> Come build with us! The BlackBerry® Developer Conference in SF, CA
>>>>>>>> is the only developer event you need to attend this year. Jumpstart your
>>>>>>>> developing skills, take BlackBerry mobile applications to market and stay
>>>>>>>> ahead of the curve. Join us from November 9-12, 2009. Register now!
>>>>>>>> http://p.sf.net/sfu/devconf
>>>>>>>> --
>>>>>>>> -----------------------------------------------
>>>>>>>> jEdit Developers' List
>>>>>>>> jEd...@li...
>>>>>>>> https://lists.sourceforge.net/lists/listinfo/jedit-devel
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
|
|
From: Shlomy R. <sre...@gm...> - 2009-10-03 16:18:30
|
It would (if possible), but that would require a larger change, which
has a potential of introducing another bug. The one I did just adds
the word "synchronized"... If there is a problem with that, I will
consider another change. I am not completely sure it can be made
reentrant, though.
Shlomy
On Sat, Oct 3, 2009 at 5:13 PM, Dale Anson <da...@gr...> wrote:
> woops, just saw your other email about TaskList.
>
> Wouldn't it be better to make TokenMarker reentrant rather than synchronized?
>
> Dale
>
>
> On Sat, Oct 3, 2009 at 9:08 AM, Dale Anson <da...@gr...> wrote:
>> What was the issue with TaskList? And which TaskList -- the one in
>> PluginManager or the one from Eric's daily builds?
>>
>> Dale
>>
>>
>> On Sat, Oct 3, 2009 at 7:39 AM, Shlomy Reinstein <sre...@gm...> wrote:
>>> I have committed this small change (make TokenMarker.markTokens
>>> synchronized) and it seems to solve the issue with TaskList.
>>> I also haven't noticed any performance impact of this. If anyone
>>> discovers a performance problem caused by this, let me know and I'll
>>> fix it.
>>>
>>> Shlomy
>>>
>>> On Sat, Oct 3, 2009 at 11:40 AM, Shlomy Reinstein <sre...@gm...> wrote:
>>>> Hi,
>>>>
>>>> It turns out that TokenMarker.markTokens is not thread-safe. It needs
>>>> to be synchronized. It has a 'context' field, that is potentially
>>>> updated each time "markTokens" is called. If called simultaneously by
>>>> multiple threads, each may modify it, causing the other threads to use
>>>> an incorrect context.
>>>>
>>>> I just verified that the problem happens very frequently with the
>>>> TaskList plugin. TaskList has a dockable which shows the tasks in the
>>>> current buffer, and also highlights tasks in the text area. To find
>>>> the tasks in the buffer, it uses "markTokens" in the background. So,
>>>> when this plugin is in use, I experience all kinds of painting issues
>>>> and occasionally the NPE that Dale experienced.
>>>> Disabling the TaskList plugin gets rid of all these problems.
>>>>
>>>> However, it is not a TaskList problem, "markTokens" simply needs to be
>>>> synchronized. If this causes some performance problem in the core,
>>>> then we should have two separate paths to markTokens, one for the core
>>>> and one for plugins, and make the plugin's path ensure thread-safety,
>>>> e.g. by deferring the task to the AWT thread.
>>>>
>>>> Shlomy
>>>>
>>>> On Sat, Oct 3, 2009 at 3:39 AM, Dale Anson <da...@gr...> wrote:
>>>>> Actually, I verified that context.parent is occasionally null, which
>>>>> causes the NPE when its 'rules' field is referenced. Thinking about
>>>>> this some more, I bet is it's a threading issue. It could be that in
>>>>> the first check (in the code you sent) it is not null, but is reset to
>>>>> null before the method is actually invoked. That would mean my patch
>>>>> is not very good since it doesn't address thread-safety.
>>>>>
>>>>> I did double check, I've got source for jEdit at revision 16233. The
>>>>> only change I have is the one I made to this class. I verified that
>>>>> I'm using the standard java mode from svn also, and it's while editing
>>>>> java files that I know I've seen the problem. I'm not sure if I've
>>>>> seen this issue in other modes.
>>>>>
>>>>> Dale
>>>>>
>>>>>
>>>>> On Fri, Oct 2, 2009 at 2:46 AM, Shlomy Reinstein <sre...@gm...> wrote:
>>>>>> I took a more thorough look at the code. It is absolutely IMPOSSIBLE
>>>>>> that context.parent is null in the method that generates the NPE for
>>>>>> you, unless you've changed something in the jedit core. There is only
>>>>>> one place where the method generating the NPE is called:
>>>>>> //{{{ check for end of delegate
>>>>>> if (context.parent != null
>>>>>> && context.parent.inRule != null
>>>>>> && checkDelegateEnd(context.parent.inRule))
>>>>>> {
>>>>>> seenWhitespaceEnd = true;
>>>>>> continue main_loop;
>>>>>> } //}}}
>>>>>>
>>>>>> Note the check for "context.parent" not being null before calling this method.
>>>>>> An NPE can still be caused by this line, but it won't be for
>>>>>> "context.parent" being null, rather it would be for
>>>>>> "context.parent.rules" being null.
>>>>>>
>>>>>> Shlomy
>>>>>>
>>>>>> On Fri, Oct 2, 2009 at 8:15 AM, Dale Anson <da...@gr...> wrote:
>>>>>>> No, I can't reproduce it reliably. Somehow, I get into this state
>>>>>>> after a couple of hours of running jEdit. Usually, it happens when
>>>>>>> I'm editing somewhere in the middle of a file, I'll hit Enter at the
>>>>>>> end of a line to start the next line and then there is the trouble. I
>>>>>>> have verified that it is definitely the case that context.parent is
>>>>>>> sometimes null. I see in LineContext itself that there are a several
>>>>>>> checks for a null parent, but not elsewhere. I think I've only seen
>>>>>>> this with java files, but I'm not absolutely certain.
>>>>>>>
>>>>>>> Do you know of a better thing to do if context.parent is null?
>>>>>>>
>>>>>>> Dale
>>>>>>>
>>>>>>>
>>>>>>> On Thu, Oct 1, 2009 at 11:31 PM, Shlomy Reinstein <sre...@gm...> wrote:
>>>>>>>> Hi,
>>>>>>>> Can you reproduce this every time with some example?
>>>>>>>> If not , does this happen when you are editing (i.e. pressing enter
>>>>>>>> on) the first or second line, or also when editing subsequent lines
>>>>>>>> down the file?
>>>>>>>> The 'parent' is usually the LineContext of the previous line.
>>>>>>>> Shlomy
>>>>>>>>
>>>>>>>> On Fri, Oct 2, 2009 at 3:33 AM, Dale Anson <da...@gr...> wrote:
>>>>>>>>> I entered a patch to fix this, see tracker 2871624. I did some more
>>>>>>>>> investigation, and it turns out that context.parent can be null. The
>>>>>>>>> patch checks for this case, and if the parent is null, it just
>>>>>>>>> continues to use the same context. So far, this seems to be working
>>>>>>>>> fine.
>>>>>>>>>
>>>>>>>>> Dale
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Wed, Sep 30, 2009 at 1:07 PM, Alan Ezust <ala...@gm...> wrote:
>>>>>>>>>> I've seen an NPE there from time to time also, but I can't reproduce it.
>>>>>>>>>>
>>>>>>>>>> On Wed, Sep 30, 2009 at 11:50 AM, Dale Anson <da...@gr...> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On Wed, Sep 30, 2009 at 9:58 AM, Matthieu Casanova
>>>>>>>>>>> <cho...@gm...> wrote:
>>>>>>>>>>> > But in that code, the fields are used as parameters, so filling the
>>>>>>>>>>> > keyword
>>>>>>>>>>> > field at line 294 is used to highlight tokens, removing that will lead
>>>>>>>>>>> > to
>>>>>>>>>>> > bugs.
>>>>>>>>>>> > For the NPE at line 294, do you know if it is because of context being
>>>>>>>>>>> > null
>>>>>>>>>>> > or context.rules ?
>>>>>>>>>>>
>>>>>>>>>>> I don't know. I didn't look into it far enough. I supposed, though,
>>>>>>>>>>> if I'm the only one seeing this issue, that means I've got something
>>>>>>>>>>> wrong somewhere.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> ------------------------------------------------------------------------------
>>>>>>>>> Come build with us! The BlackBerry® Developer Conference in SF, CA
>>>>>>>>> is the only developer event you need to attend this year. Jumpstart your
>>>>>>>>> developing skills, take BlackBerry mobile applications to market and stay
>>>>>>>>> ahead of the curve. Join us from November 9-12, 2009. Register now!
>>>>>>>>> http://p.sf.net/sfu/devconf
>>>>>>>>> --
>>>>>>>>> -----------------------------------------------
>>>>>>>>> jEdit Developers' List
>>>>>>>>> jEd...@li...
>>>>>>>>> https://lists.sourceforge.net/lists/listinfo/jedit-devel
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
> ------------------------------------------------------------------------------
> Come build with us! The BlackBerry® Developer Conference in SF, CA
> is the only developer event you need to attend this year. Jumpstart your
> developing skills, take BlackBerry mobile applications to market and stay
> ahead of the curve. Join us from November 9-12, 2009. Register now!
> http://p.sf.net/sfu/devconf
> --
> -----------------------------------------------
> jEdit Developers' List
> jEd...@li...
> https://lists.sourceforge.net/lists/listinfo/jedit-devel
>
|
|
From: Matthieu C. <cho...@gm...> - 2009-10-04 10:06:01
|
Hi in fact instead of making TokenMarker synchronized why not using method
parameters instead of fields so the TokenMarker would be threadsafe without
using synchronized code which is slow and since this code is an important
point for performances I think it should be as fast as possible
Matthieu
2009/10/3 Shlomy Reinstein <sre...@gm...>
> It would (if possible), but that would require a larger change, which
> has a potential of introducing another bug. The one I did just adds
> the word "synchronized"... If there is a problem with that, I will
> consider another change. I am not completely sure it can be made
> reentrant, though.
>
> Shlomy
>
> On Sat, Oct 3, 2009 at 5:13 PM, Dale Anson <da...@gr...> wrote:
> > woops, just saw your other email about TaskList.
> >
> > Wouldn't it be better to make TokenMarker reentrant rather than
> synchronized?
> >
> > Dale
> >
> >
> > On Sat, Oct 3, 2009 at 9:08 AM, Dale Anson <da...@gr...> wrote:
> >> What was the issue with TaskList? And which TaskList -- the one in
> >> PluginManager or the one from Eric's daily builds?
> >>
> >> Dale
> >>
> >>
> >> On Sat, Oct 3, 2009 at 7:39 AM, Shlomy Reinstein <sre...@gm...>
> wrote:
> >>> I have committed this small change (make TokenMarker.markTokens
> >>> synchronized) and it seems to solve the issue with TaskList.
> >>> I also haven't noticed any performance impact of this. If anyone
> >>> discovers a performance problem caused by this, let me know and I'll
> >>> fix it.
> >>>
> >>> Shlomy
> >>>
> >>> On Sat, Oct 3, 2009 at 11:40 AM, Shlomy Reinstein <sre...@gm...>
> wrote:
> >>>> Hi,
> >>>>
> >>>> It turns out that TokenMarker.markTokens is not thread-safe. It needs
> >>>> to be synchronized. It has a 'context' field, that is potentially
> >>>> updated each time "markTokens" is called. If called simultaneously by
> >>>> multiple threads, each may modify it, causing the other threads to use
> >>>> an incorrect context.
> >>>>
> >>>> I just verified that the problem happens very frequently with the
> >>>> TaskList plugin. TaskList has a dockable which shows the tasks in the
> >>>> current buffer, and also highlights tasks in the text area. To find
> >>>> the tasks in the buffer, it uses "markTokens" in the background. So,
> >>>> when this plugin is in use, I experience all kinds of painting issues
> >>>> and occasionally the NPE that Dale experienced.
> >>>> Disabling the TaskList plugin gets rid of all these problems.
> >>>>
> >>>> However, it is not a TaskList problem, "markTokens" simply needs to be
> >>>> synchronized. If this causes some performance problem in the core,
> >>>> then we should have two separate paths to markTokens, one for the core
> >>>> and one for plugins, and make the plugin's path ensure thread-safety,
> >>>> e.g. by deferring the task to the AWT thread.
> >>>>
> >>>> Shlomy
> >>>>
> >>>> On Sat, Oct 3, 2009 at 3:39 AM, Dale Anson <da...@gr...>
> wrote:
> >>>>> Actually, I verified that context.parent is occasionally null, which
> >>>>> causes the NPE when its 'rules' field is referenced. Thinking about
> >>>>> this some more, I bet is it's a threading issue. It could be that in
> >>>>> the first check (in the code you sent) it is not null, but is reset
> to
> >>>>> null before the method is actually invoked. That would mean my patch
> >>>>> is not very good since it doesn't address thread-safety.
> >>>>>
> >>>>> I did double check, I've got source for jEdit at revision 16233. The
> >>>>> only change I have is the one I made to this class. I verified that
> >>>>> I'm using the standard java mode from svn also, and it's while
> editing
> >>>>> java files that I know I've seen the problem. I'm not sure if I've
> >>>>> seen this issue in other modes.
> >>>>>
> >>>>> Dale
> >>>>>
> >>>>>
> >>>>> On Fri, Oct 2, 2009 at 2:46 AM, Shlomy Reinstein <sre...@gm...>
> wrote:
> >>>>>> I took a more thorough look at the code. It is absolutely IMPOSSIBLE
> >>>>>> that context.parent is null in the method that generates the NPE for
> >>>>>> you, unless you've changed something in the jedit core. There is
> only
> >>>>>> one place where the method generating the NPE is called:
> >>>>>> //{{{ check for end of delegate
> >>>>>> if (context.parent != null
> >>>>>> && context.parent.inRule != null
> >>>>>> &&
> checkDelegateEnd(context.parent.inRule))
> >>>>>> {
> >>>>>> seenWhitespaceEnd = true;
> >>>>>> continue main_loop;
> >>>>>> } //}}}
> >>>>>>
> >>>>>> Note the check for "context.parent" not being null before calling
> this method.
> >>>>>> An NPE can still be caused by this line, but it won't be for
> >>>>>> "context.parent" being null, rather it would be for
> >>>>>> "context.parent.rules" being null.
> >>>>>>
> >>>>>> Shlomy
> >>>>>>
> >>>>>> On Fri, Oct 2, 2009 at 8:15 AM, Dale Anson <da...@gr...>
> wrote:
> >>>>>>> No, I can't reproduce it reliably. Somehow, I get into this state
> >>>>>>> after a couple of hours of running jEdit. Usually, it happens when
> >>>>>>> I'm editing somewhere in the middle of a file, I'll hit Enter at
> the
> >>>>>>> end of a line to start the next line and then there is the trouble.
> I
> >>>>>>> have verified that it is definitely the case that context.parent is
> >>>>>>> sometimes null. I see in LineContext itself that there are a
> several
> >>>>>>> checks for a null parent, but not elsewhere. I think I've only
> seen
> >>>>>>> this with java files, but I'm not absolutely certain.
> >>>>>>>
> >>>>>>> Do you know of a better thing to do if context.parent is null?
> >>>>>>>
> >>>>>>> Dale
> >>>>>>>
> >>>>>>>
> >>>>>>> On Thu, Oct 1, 2009 at 11:31 PM, Shlomy Reinstein <
> sre...@gm...> wrote:
> >>>>>>>> Hi,
> >>>>>>>> Can you reproduce this every time with some example?
> >>>>>>>> If not , does this happen when you are editing (i.e. pressing
> enter
> >>>>>>>> on) the first or second line, or also when editing subsequent
> lines
> >>>>>>>> down the file?
> >>>>>>>> The 'parent' is usually the LineContext of the previous line.
> >>>>>>>> Shlomy
> >>>>>>>>
> >>>>>>>> On Fri, Oct 2, 2009 at 3:33 AM, Dale Anson <da...@gr...>
> wrote:
> >>>>>>>>> I entered a patch to fix this, see tracker 2871624. I did some
> more
> >>>>>>>>> investigation, and it turns out that context.parent can be null.
> The
> >>>>>>>>> patch checks for this case, and if the parent is null, it just
> >>>>>>>>> continues to use the same context. So far, this seems to be
> working
> >>>>>>>>> fine.
> >>>>>>>>>
> >>>>>>>>> Dale
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> On Wed, Sep 30, 2009 at 1:07 PM, Alan Ezust <
> ala...@gm...> wrote:
> >>>>>>>>>> I've seen an NPE there from time to time also, but I can't
> reproduce it.
> >>>>>>>>>>
> >>>>>>>>>> On Wed, Sep 30, 2009 at 11:50 AM, Dale Anson <
> da...@gr...> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>> On Wed, Sep 30, 2009 at 9:58 AM, Matthieu Casanova
> >>>>>>>>>>> <cho...@gm...> wrote:
> >>>>>>>>>>> > But in that code, the fields are used as parameters, so
> filling the
> >>>>>>>>>>> > keyword
> >>>>>>>>>>> > field at line 294 is used to highlight tokens, removing that
> will lead
> >>>>>>>>>>> > to
> >>>>>>>>>>> > bugs.
> >>>>>>>>>>> > For the NPE at line 294, do you know if it is because of
> context being
> >>>>>>>>>>> > null
> >>>>>>>>>>> > or context.rules ?
> >>>>>>>>>>>
> >>>>>>>>>>> I don't know. I didn't look into it far enough. I supposed,
> though,
> >>>>>>>>>>> if I'm the only one seeing this issue, that means I've got
> something
> >>>>>>>>>>> wrong somewhere.
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> ------------------------------------------------------------------------------
> >>>>>>>>> Come build with us! The BlackBerry® Developer Conference in
> SF, CA
> >>>>>>>>> is the only developer event you need to attend this year.
> Jumpstart your
> >>>>>>>>> developing skills, take BlackBerry mobile applications to market
> and stay
> >>>>>>>>> ahead of the curve. Join us from November 9-12, 2009.
> Register now!
> >>>>>>>>> http://p.sf.net/sfu/devconf
> >>>>>>>>> --
> >>>>>>>>> -----------------------------------------------
> >>>>>>>>> jEdit Developers' List
> >>>>>>>>> jEd...@li...
> >>>>>>>>> https://lists.sourceforge.net/lists/listinfo/jedit-devel
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>>
> >>
> >
> >
> ------------------------------------------------------------------------------
> > Come build with us! The BlackBerry® Developer Conference in SF, CA
> > is the only developer event you need to attend this year. Jumpstart your
> > developing skills, take BlackBerry mobile applications to market and stay
> > ahead of the curve. Join us from November 9-12, 2009. Register
> now!
> > http://p.sf.net/sfu/devconf
> > --
> > -----------------------------------------------
> > jEdit Developers' List
> > jEd...@li...
> > https://lists.sourceforge.net/lists/listinfo/jedit-devel
> >
>
>
> ------------------------------------------------------------------------------
> Come build with us! The BlackBerry® Developer Conference in SF, CA
> is the only developer event you need to attend this year. Jumpstart your
> developing skills, take BlackBerry mobile applications to market and stay
> ahead of the curve. Join us from November 9-12, 2009. Register now!
> http://p.sf.net/sfu/devconf
> --
> -----------------------------------------------
> jEdit Developers' List
> jEd...@li...
> https://lists.sourceforge.net/lists/listinfo/jedit-devel
>
|
|
From: Shlomy R. <sre...@gm...> - 2009-10-04 13:13:21
|
I agree that making TokenMarker re-entrant rather than synchronized is
better for performance. But like I wrote before, there are several
fields that are modified by markTokens and its callees, and making it
re-entrant is a larger change which might introduce another bug. If
you'd like to do this yourself, feel free to...
Unless you really can show a slowness in jEdit as a result of this,
let's stay with the simple and safe fix. It fixes a real issue and I
don't see it causing any.
Shlomy
On Sun, Oct 4, 2009 at 12:05 PM, Matthieu Casanova
<cho...@gm...> wrote:
> Hi in fact instead of making TokenMarker synchronized why not using method
> parameters instead of fields so the TokenMarker would be threadsafe without
> using synchronized code which is slow and since this code is an important
> point for performances I think it should be as fast as possible
> Matthieu
>
> 2009/10/3 Shlomy Reinstein <sre...@gm...>
>>
>> It would (if possible), but that would require a larger change, which
>> has a potential of introducing another bug. The one I did just adds
>> the word "synchronized"... If there is a problem with that, I will
>> consider another change. I am not completely sure it can be made
>> reentrant, though.
>>
>> Shlomy
>>
>> On Sat, Oct 3, 2009 at 5:13 PM, Dale Anson <da...@gr...> wrote:
>> > woops, just saw your other email about TaskList.
>> >
>> > Wouldn't it be better to make TokenMarker reentrant rather than
>> > synchronized?
>> >
>> > Dale
>> >
>> >
>> > On Sat, Oct 3, 2009 at 9:08 AM, Dale Anson <da...@gr...> wrote:
>> >> What was the issue with TaskList? And which TaskList -- the one in
>> >> PluginManager or the one from Eric's daily builds?
>> >>
>> >> Dale
>> >>
>> >>
>> >> On Sat, Oct 3, 2009 at 7:39 AM, Shlomy Reinstein <sre...@gm...>
>> >> wrote:
>> >>> I have committed this small change (make TokenMarker.markTokens
>> >>> synchronized) and it seems to solve the issue with TaskList.
>> >>> I also haven't noticed any performance impact of this. If anyone
>> >>> discovers a performance problem caused by this, let me know and I'll
>> >>> fix it.
>> >>>
>> >>> Shlomy
>> >>>
>> >>> On Sat, Oct 3, 2009 at 11:40 AM, Shlomy Reinstein <sre...@gm...>
>> >>> wrote:
>> >>>> Hi,
>> >>>>
>> >>>> It turns out that TokenMarker.markTokens is not thread-safe. It needs
>> >>>> to be synchronized. It has a 'context' field, that is potentially
>> >>>> updated each time "markTokens" is called. If called simultaneously by
>> >>>> multiple threads, each may modify it, causing the other threads to
>> >>>> use
>> >>>> an incorrect context.
>> >>>>
>> >>>> I just verified that the problem happens very frequently with the
>> >>>> TaskList plugin. TaskList has a dockable which shows the tasks in the
>> >>>> current buffer, and also highlights tasks in the text area. To find
>> >>>> the tasks in the buffer, it uses "markTokens" in the background. So,
>> >>>> when this plugin is in use, I experience all kinds of painting issues
>> >>>> and occasionally the NPE that Dale experienced.
>> >>>> Disabling the TaskList plugin gets rid of all these problems.
>> >>>>
>> >>>> However, it is not a TaskList problem, "markTokens" simply needs to
>> >>>> be
>> >>>> synchronized. If this causes some performance problem in the core,
>> >>>> then we should have two separate paths to markTokens, one for the
>> >>>> core
>> >>>> and one for plugins, and make the plugin's path ensure thread-safety,
>> >>>> e.g. by deferring the task to the AWT thread.
>> >>>>
>> >>>> Shlomy
>> >>>>
>> >>>> On Sat, Oct 3, 2009 at 3:39 AM, Dale Anson <da...@gr...>
>> >>>> wrote:
>> >>>>> Actually, I verified that context.parent is occasionally null, which
>> >>>>> causes the NPE when its 'rules' field is referenced. Thinking about
>> >>>>> this some more, I bet is it's a threading issue. It could be that
>> >>>>> in
>> >>>>> the first check (in the code you sent) it is not null, but is reset
>> >>>>> to
>> >>>>> null before the method is actually invoked. That would mean my
>> >>>>> patch
>> >>>>> is not very good since it doesn't address thread-safety.
>> >>>>>
>> >>>>> I did double check, I've got source for jEdit at revision 16233.
>> >>>>> The
>> >>>>> only change I have is the one I made to this class. I verified that
>> >>>>> I'm using the standard java mode from svn also, and it's while
>> >>>>> editing
>> >>>>> java files that I know I've seen the problem. I'm not sure if I've
>> >>>>> seen this issue in other modes.
>> >>>>>
>> >>>>> Dale
>> >>>>>
>> >>>>>
>> >>>>> On Fri, Oct 2, 2009 at 2:46 AM, Shlomy Reinstein
>> >>>>> <sre...@gm...> wrote:
>> >>>>>> I took a more thorough look at the code. It is absolutely
>> >>>>>> IMPOSSIBLE
>> >>>>>> that context.parent is null in the method that generates the NPE
>> >>>>>> for
>> >>>>>> you, unless you've changed something in the jedit core. There is
>> >>>>>> only
>> >>>>>> one place where the method generating the NPE is called:
>> >>>>>> //{{{ check for end of delegate
>> >>>>>> if (context.parent != null
>> >>>>>> && context.parent.inRule != null
>> >>>>>> &&
>> >>>>>> checkDelegateEnd(context.parent.inRule))
>> >>>>>> {
>> >>>>>> seenWhitespaceEnd = true;
>> >>>>>> continue main_loop;
>> >>>>>> } //}}}
>> >>>>>>
>> >>>>>> Note the check for "context.parent" not being null before calling
>> >>>>>> this method.
>> >>>>>> An NPE can still be caused by this line, but it won't be for
>> >>>>>> "context.parent" being null, rather it would be for
>> >>>>>> "context.parent.rules" being null.
>> >>>>>>
>> >>>>>> Shlomy
>> >>>>>>
>> >>>>>> On Fri, Oct 2, 2009 at 8:15 AM, Dale Anson <da...@gr...>
>> >>>>>> wrote:
>> >>>>>>> No, I can't reproduce it reliably. Somehow, I get into this state
>> >>>>>>> after a couple of hours of running jEdit. Usually, it happens
>> >>>>>>> when
>> >>>>>>> I'm editing somewhere in the middle of a file, I'll hit Enter at
>> >>>>>>> the
>> >>>>>>> end of a line to start the next line and then there is the
>> >>>>>>> trouble. I
>> >>>>>>> have verified that it is definitely the case that context.parent
>> >>>>>>> is
>> >>>>>>> sometimes null. I see in LineContext itself that there are a
>> >>>>>>> several
>> >>>>>>> checks for a null parent, but not elsewhere. I think I've only
>> >>>>>>> seen
>> >>>>>>> this with java files, but I'm not absolutely certain.
>> >>>>>>>
>> >>>>>>> Do you know of a better thing to do if context.parent is null?
>> >>>>>>>
>> >>>>>>> Dale
>> >>>>>>>
>> >>>>>>>
>> >>>>>>> On Thu, Oct 1, 2009 at 11:31 PM, Shlomy Reinstein
>> >>>>>>> <sre...@gm...> wrote:
>> >>>>>>>> Hi,
>> >>>>>>>> Can you reproduce this every time with some example?
>> >>>>>>>> If not , does this happen when you are editing (i.e. pressing
>> >>>>>>>> enter
>> >>>>>>>> on) the first or second line, or also when editing subsequent
>> >>>>>>>> lines
>> >>>>>>>> down the file?
>> >>>>>>>> The 'parent' is usually the LineContext of the previous line.
>> >>>>>>>> Shlomy
>> >>>>>>>>
>> >>>>>>>> On Fri, Oct 2, 2009 at 3:33 AM, Dale Anson <da...@gr...>
>> >>>>>>>> wrote:
>> >>>>>>>>> I entered a patch to fix this, see tracker 2871624. I did some
>> >>>>>>>>> more
>> >>>>>>>>> investigation, and it turns out that context.parent can be null.
>> >>>>>>>>> The
>> >>>>>>>>> patch checks for this case, and if the parent is null, it just
>> >>>>>>>>> continues to use the same context. So far, this seems to be
>> >>>>>>>>> working
>> >>>>>>>>> fine.
>> >>>>>>>>>
>> >>>>>>>>> Dale
>> >>>>>>>>>
>> >>>>>>>>>
>> >>>>>>>>> On Wed, Sep 30, 2009 at 1:07 PM, Alan Ezust
>> >>>>>>>>> <ala...@gm...> wrote:
>> >>>>>>>>>> I've seen an NPE there from time to time also, but I can't
>> >>>>>>>>>> reproduce it.
>> >>>>>>>>>>
>> >>>>>>>>>> On Wed, Sep 30, 2009 at 11:50 AM, Dale Anson
>> >>>>>>>>>> <da...@gr...> wrote:
>> >>>>>>>>>>>
>> >>>>>>>>>>> On Wed, Sep 30, 2009 at 9:58 AM, Matthieu Casanova
>> >>>>>>>>>>> <cho...@gm...> wrote:
>> >>>>>>>>>>> > But in that code, the fields are used as parameters, so
>> >>>>>>>>>>> > filling the
>> >>>>>>>>>>> > keyword
>> >>>>>>>>>>> > field at line 294 is used to highlight tokens, removing that
>> >>>>>>>>>>> > will lead
>> >>>>>>>>>>> > to
>> >>>>>>>>>>> > bugs.
>> >>>>>>>>>>> > For the NPE at line 294, do you know if it is because of
>> >>>>>>>>>>> > context being
>> >>>>>>>>>>> > null
>> >>>>>>>>>>> > or context.rules ?
>> >>>>>>>>>>>
>> >>>>>>>>>>> I don't know. I didn't look into it far enough. I supposed,
>> >>>>>>>>>>> though,
>> >>>>>>>>>>> if I'm the only one seeing this issue, that means I've got
>> >>>>>>>>>>> something
>> >>>>>>>>>>> wrong somewhere.
>> >>>>>>>>>>>
>> >>>>>>>>>>
>> >>>>>>>>>>
>> >>>>>>>>>
>> >>>>>>>>>
>> >>>>>>>>> ------------------------------------------------------------------------------
>> >>>>>>>>> Come build with us! The BlackBerry® Developer Conference in
>> >>>>>>>>> SF, CA
>> >>>>>>>>> is the only developer event you need to attend this year.
>> >>>>>>>>> Jumpstart your
>> >>>>>>>>> developing skills, take BlackBerry mobile applications to market
>> >>>>>>>>> and stay
>> >>>>>>>>> ahead of the curve. Join us from November 9-12, 2009.
>> >>>>>>>>> Register now!
>> >>>>>>>>> http://p.sf.net/sfu/devconf
>> >>>>>>>>> --
>> >>>>>>>>> -----------------------------------------------
>> >>>>>>>>> jEdit Developers' List
>> >>>>>>>>> jEd...@li...
>> >>>>>>>>> https://lists.sourceforge.net/lists/listinfo/jedit-devel
>> >>>>>>>>>
>> >>>>>>>>
>> >>>>>>>
>> >>>>>>
>> >>>>>
>> >>>>
>> >>>
>> >>
>> >
>> >
>> > ------------------------------------------------------------------------------
>> > Come build with us! The BlackBerry® Developer Conference in SF, CA
>> > is the only developer event you need to attend this year. Jumpstart your
>> > developing skills, take BlackBerry mobile applications to market and
>> > stay
>> > ahead of the curve. Join us from November 9-12, 2009. Register
>> > now!
>> > http://p.sf.net/sfu/devconf
>> > --
>> > -----------------------------------------------
>> > jEdit Developers' List
>> > jEd...@li...
>> > https://lists.sourceforge.net/lists/listinfo/jedit-devel
>> >
>>
>>
>> ------------------------------------------------------------------------------
>> Come build with us! The BlackBerry® Developer Conference in SF, CA
>> is the only developer event you need to attend this year. Jumpstart your
>> developing skills, take BlackBerry mobile applications to market and stay
>> ahead of the curve. Join us from November 9-12, 2009. Register
>> now!
>> http://p.sf.net/sfu/devconf
>> --
>> -----------------------------------------------
>> jEdit Developers' List
>> jEd...@li...
>> https://lists.sourceforge.net/lists/listinfo/jedit-devel
>
>
|
|
From: Dale A. <da...@gr...> - 2009-10-04 13:42:36
|
Making markTokens re-entrant would help performance in TaskList also.
TaskList uses a synchronized method to find tasks, the method is
synchronized to get around threading issues. I think if markTokens
was re-entrant, this TaskList method would not need to be
synchronized.
It is more work to make markTokens re-entrant, but it looks to be
straight-forward to do. I'll give it a shot today and pass on the
results for review.
Dale
On Sun, Oct 4, 2009 at 7:13 AM, Shlomy Reinstein <sre...@gm...> wrote:
> I agree that making TokenMarker re-entrant rather than synchronized is
> better for performance. But like I wrote before, there are several
> fields that are modified by markTokens and its callees, and making it
> re-entrant is a larger change which might introduce another bug. If
> you'd like to do this yourself, feel free to...
>
> Unless you really can show a slowness in jEdit as a result of this,
> let's stay with the simple and safe fix. It fixes a real issue and I
> don't see it causing any.
>
> Shlomy
>
> On Sun, Oct 4, 2009 at 12:05 PM, Matthieu Casanova
> <cho...@gm...> wrote:
>> Hi in fact instead of making TokenMarker synchronized why not using method
>> parameters instead of fields so the TokenMarker would be threadsafe without
>> using synchronized code which is slow and since this code is an important
>> point for performances I think it should be as fast as possible
>> Matthieu
>>
>> 2009/10/3 Shlomy Reinstein <sre...@gm...>
>>>
>>> It would (if possible), but that would require a larger change, which
>>> has a potential of introducing another bug. The one I did just adds
>>> the word "synchronized"... If there is a problem with that, I will
>>> consider another change. I am not completely sure it can be made
>>> reentrant, though.
>>>
>>> Shlomy
>>>
>>> On Sat, Oct 3, 2009 at 5:13 PM, Dale Anson <da...@gr...> wrote:
>>> > woops, just saw your other email about TaskList.
>>> >
>>> > Wouldn't it be better to make TokenMarker reentrant rather than
>>> > synchronized?
>>> >
>>> > Dale
>>> >
>>> >
>>> > On Sat, Oct 3, 2009 at 9:08 AM, Dale Anson <da...@gr...> wrote:
>>> >> What was the issue with TaskList? And which TaskList -- the one in
>>> >> PluginManager or the one from Eric's daily builds?
>>> >>
>>> >> Dale
>>> >>
>>> >>
>>> >> On Sat, Oct 3, 2009 at 7:39 AM, Shlomy Reinstein <sre...@gm...>
>>> >> wrote:
>>> >>> I have committed this small change (make TokenMarker.markTokens
>>> >>> synchronized) and it seems to solve the issue with TaskList.
>>> >>> I also haven't noticed any performance impact of this. If anyone
>>> >>> discovers a performance problem caused by this, let me know and I'll
>>> >>> fix it.
>>> >>>
>>> >>> Shlomy
>>> >>>
>>> >>> On Sat, Oct 3, 2009 at 11:40 AM, Shlomy Reinstein <sre...@gm...>
>>> >>> wrote:
>>> >>>> Hi,
>>> >>>>
>>> >>>> It turns out that TokenMarker.markTokens is not thread-safe. It needs
>>> >>>> to be synchronized. It has a 'context' field, that is potentially
>>> >>>> updated each time "markTokens" is called. If called simultaneously by
>>> >>>> multiple threads, each may modify it, causing the other threads to
>>> >>>> use
>>> >>>> an incorrect context.
>>> >>>>
>>> >>>> I just verified that the problem happens very frequently with the
>>> >>>> TaskList plugin. TaskList has a dockable which shows the tasks in the
>>> >>>> current buffer, and also highlights tasks in the text area. To find
>>> >>>> the tasks in the buffer, it uses "markTokens" in the background. So,
>>> >>>> when this plugin is in use, I experience all kinds of painting issues
>>> >>>> and occasionally the NPE that Dale experienced.
>>> >>>> Disabling the TaskList plugin gets rid of all these problems.
>>> >>>>
>>> >>>> However, it is not a TaskList problem, "markTokens" simply needs to
>>> >>>> be
>>> >>>> synchronized. If this causes some performance problem in the core,
>>> >>>> then we should have two separate paths to markTokens, one for the
>>> >>>> core
>>> >>>> and one for plugins, and make the plugin's path ensure thread-safety,
>>> >>>> e.g. by deferring the task to the AWT thread.
>>> >>>>
>>> >>>> Shlomy
>>> >>>>
>>> >>>> On Sat, Oct 3, 2009 at 3:39 AM, Dale Anson <da...@gr...>
>>> >>>> wrote:
>>> >>>>> Actually, I verified that context.parent is occasionally null, which
>>> >>>>> causes the NPE when its 'rules' field is referenced. Thinking about
>>> >>>>> this some more, I bet is it's a threading issue. It could be that
>>> >>>>> in
>>> >>>>> the first check (in the code you sent) it is not null, but is reset
>>> >>>>> to
>>> >>>>> null before the method is actually invoked. That would mean my
>>> >>>>> patch
>>> >>>>> is not very good since it doesn't address thread-safety.
>>> >>>>>
>>> >>>>> I did double check, I've got source for jEdit at revision 16233.
>>> >>>>> The
>>> >>>>> only change I have is the one I made to this class. I verified that
>>> >>>>> I'm using the standard java mode from svn also, and it's while
>>> >>>>> editing
>>> >>>>> java files that I know I've seen the problem. I'm not sure if I've
>>> >>>>> seen this issue in other modes.
>>> >>>>>
>>> >>>>> Dale
>>> >>>>>
>>> >>>>>
>>> >>>>> On Fri, Oct 2, 2009 at 2:46 AM, Shlomy Reinstein
>>> >>>>> <sre...@gm...> wrote:
>>> >>>>>> I took a more thorough look at the code. It is absolutely
>>> >>>>>> IMPOSSIBLE
>>> >>>>>> that context.parent is null in the method that generates the NPE
>>> >>>>>> for
>>> >>>>>> you, unless you've changed something in the jedit core. There is
>>> >>>>>> only
>>> >>>>>> one place where the method generating the NPE is called:
>>> >>>>>> //{{{ check for end of delegate
>>> >>>>>> if (context.parent != null
>>> >>>>>> && context.parent.inRule != null
>>> >>>>>> &&
>>> >>>>>> checkDelegateEnd(context.parent.inRule))
>>> >>>>>> {
>>> >>>>>> seenWhitespaceEnd = true;
>>> >>>>>> continue main_loop;
>>> >>>>>> } //}}}
>>> >>>>>>
>>> >>>>>> Note the check for "context.parent" not being null before calling
>>> >>>>>> this method.
>>> >>>>>> An NPE can still be caused by this line, but it won't be for
>>> >>>>>> "context.parent" being null, rather it would be for
>>> >>>>>> "context.parent.rules" being null.
>>> >>>>>>
>>> >>>>>> Shlomy
>>> >>>>>>
>>> >>>>>> On Fri, Oct 2, 2009 at 8:15 AM, Dale Anson <da...@gr...>
>>> >>>>>> wrote:
>>> >>>>>>> No, I can't reproduce it reliably. Somehow, I get into this state
>>> >>>>>>> after a couple of hours of running jEdit. Usually, it happens
>>> >>>>>>> when
>>> >>>>>>> I'm editing somewhere in the middle of a file, I'll hit Enter at
>>> >>>>>>> the
>>> >>>>>>> end of a line to start the next line and then there is the
>>> >>>>>>> trouble. I
>>> >>>>>>> have verified that it is definitely the case that context.parent
>>> >>>>>>> is
>>> >>>>>>> sometimes null. I see in LineContext itself that there are a
>>> >>>>>>> several
>>> >>>>>>> checks for a null parent, but not elsewhere. I think I've only
>>> >>>>>>> seen
>>> >>>>>>> this with java files, but I'm not absolutely certain.
>>> >>>>>>>
>>> >>>>>>> Do you know of a better thing to do if context.parent is null?
>>> >>>>>>>
>>> >>>>>>> Dale
>>> >>>>>>>
>>> >>>>>>>
>>> >>>>>>> On Thu, Oct 1, 2009 at 11:31 PM, Shlomy Reinstein
>>> >>>>>>> <sre...@gm...> wrote:
>>> >>>>>>>> Hi,
>>> >>>>>>>> Can you reproduce this every time with some example?
>>> >>>>>>>> If not , does this happen when you are editing (i.e. pressing
>>> >>>>>>>> enter
>>> >>>>>>>> on) the first or second line, or also when editing subsequent
>>> >>>>>>>> lines
>>> >>>>>>>> down the file?
>>> >>>>>>>> The 'parent' is usually the LineContext of the previous line.
>>> >>>>>>>> Shlomy
>>> >>>>>>>>
>>> >>>>>>>> On Fri, Oct 2, 2009 at 3:33 AM, Dale Anson <da...@gr...>
>>> >>>>>>>> wrote:
>>> >>>>>>>>> I entered a patch to fix this, see tracker 2871624. I did some
>>> >>>>>>>>> more
>>> >>>>>>>>> investigation, and it turns out that context.parent can be null.
>>> >>>>>>>>> The
>>> >>>>>>>>> patch checks for this case, and if the parent is null, it just
>>> >>>>>>>>> continues to use the same context. So far, this seems to be
>>> >>>>>>>>> working
>>> >>>>>>>>> fine.
>>> >>>>>>>>>
>>> >>>>>>>>> Dale
>>> >>>>>>>>>
>>> >>>>>>>>>
>>> >>>>>>>>> On Wed, Sep 30, 2009 at 1:07 PM, Alan Ezust
>>> >>>>>>>>> <ala...@gm...> wrote:
>>> >>>>>>>>>> I've seen an NPE there from time to time also, but I can't
>>> >>>>>>>>>> reproduce it.
>>> >>>>>>>>>>
>>> >>>>>>>>>> On Wed, Sep 30, 2009 at 11:50 AM, Dale Anson
>>> >>>>>>>>>> <da...@gr...> wrote:
>>> >>>>>>>>>>>
>>> >>>>>>>>>>> On Wed, Sep 30, 2009 at 9:58 AM, Matthieu Casanova
>>> >>>>>>>>>>> <cho...@gm...> wrote:
>>> >>>>>>>>>>> > But in that code, the fields are used as parameters, so
>>> >>>>>>>>>>> > filling the
>>> >>>>>>>>>>> > keyword
>>> >>>>>>>>>>> > field at line 294 is used to highlight tokens, removing that
>>> >>>>>>>>>>> > will lead
>>> >>>>>>>>>>> > to
>>> >>>>>>>>>>> > bugs.
>>> >>>>>>>>>>> > For the NPE at line 294, do you know if it is because of
>>> >>>>>>>>>>> > context being
>>> >>>>>>>>>>> > null
>>> >>>>>>>>>>> > or context.rules ?
>>> >>>>>>>>>>>
>>> >>>>>>>>>>> I don't know. I didn't look into it far enough. I supposed,
>>> >>>>>>>>>>> though,
>>> >>>>>>>>>>> if I'm the only one seeing this issue, that means I've got
>>> >>>>>>>>>>> something
>>> >>>>>>>>>>> wrong somewhere.
>>> >>>>>>>>>>>
>>> >>>>>>>>>>
>>> >>>>>>>>>>
>>> >>>>>>>>>
>>> >>>>>>>>>
>>> >>>>>>>>> ------------------------------------------------------------------------------
>>> >>>>>>>>> Come build with us! The BlackBerry® Developer Conference in
>>> >>>>>>>>> SF, CA
>>> >>>>>>>>> is the only developer event you need to attend this year.
>>> >>>>>>>>> Jumpstart your
>>> >>>>>>>>> developing skills, take BlackBerry mobile applications to market
>>> >>>>>>>>> and stay
>>> >>>>>>>>> ahead of the curve. Join us from November 9-12, 2009.
>>> >>>>>>>>> Register now!
>>> >>>>>>>>> http://p.sf.net/sfu/devconf
>>> >>>>>>>>> --
>>> >>>>>>>>> -----------------------------------------------
>>> >>>>>>>>> jEdit Developers' List
>>> >>>>>>>>> jEd...@li...
>>> >>>>>>>>> https://lists.sourceforge.net/lists/listinfo/jedit-devel
>>> >>>>>>>>>
>>> >>>>>>>>
>>> >>>>>>>
>>> >>>>>>
>>> >>>>>
>>> >>>>
>>> >>>
>>> >>
>>> >
>>> >
>>> > ------------------------------------------------------------------------------
>>> > Come build with us! The BlackBerry® Developer Conference in SF, CA
>>> > is the only developer event you need to attend this year. Jumpstart your
>>> > developing skills, take BlackBerry mobile applications to market and
>>> > stay
>>> > ahead of the curve. Join us from November 9-12, 2009. Register
>>> > now!
>>> > http://p.sf.net/sfu/devconf
>>> > --
>>> > -----------------------------------------------
>>> > jEdit Developers' List
>>> > jEd...@li...
>>> > https://lists.sourceforge.net/lists/listinfo/jedit-devel
>>> >
>>>
>>>
>>> ------------------------------------------------------------------------------
>>> Come build with us! The BlackBerry® Developer Conference in SF, CA
>>> is the only developer event you need to attend this year. Jumpstart your
>>> developing skills, take BlackBerry mobile applications to market and stay
>>> ahead of the curve. Join us from November 9-12, 2009. Register
>>> now!
>>> http://p.sf.net/sfu/devconf
>>> --
>>> -----------------------------------------------
>>> jEdit Developers' List
>>> jEd...@li...
>>> https://lists.sourceforge.net/lists/listinfo/jedit-devel
>>
>>
>
|
|
From: Dale A. <da...@gr...> - 2009-10-05 15:34:35
Attachments:
TokenMarker.java
|
Please see attached. I attached the whole file rather than a patch
since the patch turned out to be quite large. There are a lot of
small changes. Essentially, I turned the instance fields of
TokenMarker into a data object, and am passing that data object
around. Each call to markTokens gets its own data object, which keeps
one call from interfering with the values being used from another
call. I've been looking into how to write a unit test markTokens, and
would like to get that done before committing a change like this.
Dale
On Sun, Oct 4, 2009 at 7:42 AM, Dale Anson <da...@gr...> wrote:
> Making markTokens re-entrant would help performance in TaskList also.
> TaskList uses a synchronized method to find tasks, the method is
> synchronized to get around threading issues. I think if markTokens
> was re-entrant, this TaskList method would not need to be
> synchronized.
>
> It is more work to make markTokens re-entrant, but it looks to be
> straight-forward to do. I'll give it a shot today and pass on the
> results for review.
>
> Dale
>
>
> On Sun, Oct 4, 2009 at 7:13 AM, Shlomy Reinstein <sre...@gm...> wrote:
>> I agree that making TokenMarker re-entrant rather than synchronized is
>> better for performance. But like I wrote before, there are several
>> fields that are modified by markTokens and its callees, and making it
>> re-entrant is a larger change which might introduce another bug. If
>> you'd like to do this yourself, feel free to...
>>
>> Unless you really can show a slowness in jEdit as a result of this,
>> let's stay with the simple and safe fix. It fixes a real issue and I
>> don't see it causing any.
>>
>> Shlomy
>>
>> On Sun, Oct 4, 2009 at 12:05 PM, Matthieu Casanova
>> <cho...@gm...> wrote:
>>> Hi in fact instead of making TokenMarker synchronized why not using method
>>> parameters instead of fields so the TokenMarker would be threadsafe without
>>> using synchronized code which is slow and since this code is an important
>>> point for performances I think it should be as fast as possible
>>> Matthieu
>>>
>>> 2009/10/3 Shlomy Reinstein <sre...@gm...>
>>>>
>>>> It would (if possible), but that would require a larger change, which
>>>> has a potential of introducing another bug. The one I did just adds
>>>> the word "synchronized"... If there is a problem with that, I will
>>>> consider another change. I am not completely sure it can be made
>>>> reentrant, though.
>>>>
>>>> Shlomy
>>>>
>>>> On Sat, Oct 3, 2009 at 5:13 PM, Dale Anson <da...@gr...> wrote:
>>>> > woops, just saw your other email about TaskList.
>>>> >
>>>> > Wouldn't it be better to make TokenMarker reentrant rather than
>>>> > synchronized?
>>>> >
>>>> > Dale
>>>> >
>>>> >
>>>> > On Sat, Oct 3, 2009 at 9:08 AM, Dale Anson <da...@gr...> wrote:
>>>> >> What was the issue with TaskList? And which TaskList -- the one in
>>>> >> PluginManager or the one from Eric's daily builds?
>>>> >>
>>>> >> Dale
>>>> >>
>>>> >>
>>>> >> On Sat, Oct 3, 2009 at 7:39 AM, Shlomy Reinstein <sre...@gm...>
>>>> >> wrote:
>>>> >>> I have committed this small change (make TokenMarker.markTokens
>>>> >>> synchronized) and it seems to solve the issue with TaskList.
>>>> >>> I also haven't noticed any performance impact of this. If anyone
>>>> >>> discovers a performance problem caused by this, let me know and I'll
>>>> >>> fix it.
>>>> >>>
>>>> >>> Shlomy
>>>> >>>
>>>> >>> On Sat, Oct 3, 2009 at 11:40 AM, Shlomy Reinstein <sre...@gm...>
>>>> >>> wrote:
>>>> >>>> Hi,
>>>> >>>>
>>>> >>>> It turns out that TokenMarker.markTokens is not thread-safe. It needs
>>>> >>>> to be synchronized. It has a 'context' field, that is potentially
>>>> >>>> updated each time "markTokens" is called. If called simultaneously by
>>>> >>>> multiple threads, each may modify it, causing the other threads to
>>>> >>>> use
>>>> >>>> an incorrect context.
>>>> >>>>
>>>> >>>> I just verified that the problem happens very frequently with the
>>>> >>>> TaskList plugin. TaskList has a dockable which shows the tasks in the
>>>> >>>> current buffer, and also highlights tasks in the text area. To find
>>>> >>>> the tasks in the buffer, it uses "markTokens" in the background. So,
>>>> >>>> when this plugin is in use, I experience all kinds of painting issues
>>>> >>>> and occasionally the NPE that Dale experienced.
>>>> >>>> Disabling the TaskList plugin gets rid of all these problems.
>>>> >>>>
>>>> >>>> However, it is not a TaskList problem, "markTokens" simply needs to
>>>> >>>> be
>>>> >>>> synchronized. If this causes some performance problem in the core,
>>>> >>>> then we should have two separate paths to markTokens, one for the
>>>> >>>> core
>>>> >>>> and one for plugins, and make the plugin's path ensure thread-safety,
>>>> >>>> e.g. by deferring the task to the AWT thread.
>>>> >>>>
>>>> >>>> Shlomy
>>>> >>>>
>>>> >>>> On Sat, Oct 3, 2009 at 3:39 AM, Dale Anson <da...@gr...>
>>>> >>>> wrote:
>>>> >>>>> Actually, I verified that context.parent is occasionally null, which
>>>> >>>>> causes the NPE when its 'rules' field is referenced. Thinking about
>>>> >>>>> this some more, I bet is it's a threading issue. It could be that
>>>> >>>>> in
>>>> >>>>> the first check (in the code you sent) it is not null, but is reset
>>>> >>>>> to
>>>> >>>>> null before the method is actually invoked. That would mean my
>>>> >>>>> patch
>>>> >>>>> is not very good since it doesn't address thread-safety.
>>>> >>>>>
>>>> >>>>> I did double check, I've got source for jEdit at revision 16233.
>>>> >>>>> The
>>>> >>>>> only change I have is the one I made to this class. I verified that
>>>> >>>>> I'm using the standard java mode from svn also, and it's while
>>>> >>>>> editing
>>>> >>>>> java files that I know I've seen the problem. I'm not sure if I've
>>>> >>>>> seen this issue in other modes.
>>>> >>>>>
>>>> >>>>> Dale
>>>> >>>>>
>>>> >>>>>
>>>> >>>>> On Fri, Oct 2, 2009 at 2:46 AM, Shlomy Reinstein
>>>> >>>>> <sre...@gm...> wrote:
>>>> >>>>>> I took a more thorough look at the code. It is absolutely
>>>> >>>>>> IMPOSSIBLE
>>>> >>>>>> that context.parent is null in the method that generates the NPE
>>>> >>>>>> for
>>>> >>>>>> you, unless you've changed something in the jedit core. There is
>>>> >>>>>> only
>>>> >>>>>> one place where the method generating the NPE is called:
>>>> >>>>>> //{{{ check for end of delegate
>>>> >>>>>> if (context.parent != null
>>>> >>>>>> && context.parent.inRule != null
>>>> >>>>>> &&
>>>> >>>>>> checkDelegateEnd(context.parent.inRule))
>>>> >>>>>> {
>>>> >>>>>> seenWhitespaceEnd = true;
>>>> >>>>>> continue main_loop;
>>>> >>>>>> } //}}}
>>>> >>>>>>
>>>> >>>>>> Note the check for "context.parent" not being null before calling
>>>> >>>>>> this method.
>>>> >>>>>> An NPE can still be caused by this line, but it won't be for
>>>> >>>>>> "context.parent" being null, rather it would be for
>>>> >>>>>> "context.parent.rules" being null.
>>>> >>>>>>
>>>> >>>>>> Shlomy
>>>> >>>>>>
>>>> >>>>>> On Fri, Oct 2, 2009 at 8:15 AM, Dale Anson <da...@gr...>
>>>> >>>>>> wrote:
>>>> >>>>>>> No, I can't reproduce it reliably. Somehow, I get into this state
>>>> >>>>>>> after a couple of hours of running jEdit. Usually, it happens
>>>> >>>>>>> when
>>>> >>>>>>> I'm editing somewhere in the middle of a file, I'll hit Enter at
>>>> >>>>>>> the
>>>> >>>>>>> end of a line to start the next line and then there is the
>>>> >>>>>>> trouble. I
>>>> >>>>>>> have verified that it is definitely the case that context.parent
>>>> >>>>>>> is
>>>> >>>>>>> sometimes null. I see in LineContext itself that there are a
>>>> >>>>>>> several
>>>> >>>>>>> checks for a null parent, but not elsewhere. I think I've only
>>>> >>>>>>> seen
>>>> >>>>>>> this with java files, but I'm not absolutely certain.
>>>> >>>>>>>
>>>> >>>>>>> Do you know of a better thing to do if context.parent is null?
>>>> >>>>>>>
>>>> >>>>>>> Dale
>>>> >>>>>>>
>>>> >>>>>>>
>>>> >>>>>>> On Thu, Oct 1, 2009 at 11:31 PM, Shlomy Reinstein
>>>> >>>>>>> <sre...@gm...> wrote:
>>>> >>>>>>>> Hi,
>>>> >>>>>>>> Can you reproduce this every time with some example?
>>>> >>>>>>>> If not , does this happen when you are editing (i.e. pressing
>>>> >>>>>>>> enter
>>>> >>>>>>>> on) the first or second line, or also when editing subsequent
>>>> >>>>>>>> lines
>>>> >>>>>>>> down the file?
>>>> >>>>>>>> The 'parent' is usually the LineContext of the previous line.
>>>> >>>>>>>> Shlomy
>>>> >>>>>>>>
>>>> >>>>>>>> On Fri, Oct 2, 2009 at 3:33 AM, Dale Anson <da...@gr...>
>>>> >>>>>>>> wrote:
>>>> >>>>>>>>> I entered a patch to fix this, see tracker 2871624. I did some
>>>> >>>>>>>>> more
>>>> >>>>>>>>> investigation, and it turns out that context.parent can be null.
>>>> >>>>>>>>> The
>>>> >>>>>>>>> patch checks for this case, and if the parent is null, it just
>>>> >>>>>>>>> continues to use the same context. So far, this seems to be
>>>> >>>>>>>>> working
>>>> >>>>>>>>> fine.
>>>> >>>>>>>>>
>>>> >>>>>>>>> Dale
>>>> >>>>>>>>>
>>>> >>>>>>>>>
>>>> >>>>>>>>> On Wed, Sep 30, 2009 at 1:07 PM, Alan Ezust
>>>> >>>>>>>>> <ala...@gm...> wrote:
>>>> >>>>>>>>>> I've seen an NPE there from time to time also, but I can't
>>>> >>>>>>>>>> reproduce it.
>>>> >>>>>>>>>>
>>>> >>>>>>>>>> On Wed, Sep 30, 2009 at 11:50 AM, Dale Anson
>>>> >>>>>>>>>> <da...@gr...> wrote:
>>>> >>>>>>>>>>>
>>>> >>>>>>>>>>> On Wed, Sep 30, 2009 at 9:58 AM, Matthieu Casanova
>>>> >>>>>>>>>>> <cho...@gm...> wrote:
>>>> >>>>>>>>>>> > But in that code, the fields are used as parameters, so
>>>> >>>>>>>>>>> > filling the
>>>> >>>>>>>>>>> > keyword
>>>> >>>>>>>>>>> > field at line 294 is used to highlight tokens, removing that
>>>> >>>>>>>>>>> > will lead
>>>> >>>>>>>>>>> > to
>>>> >>>>>>>>>>> > bugs.
>>>> >>>>>>>>>>> > For the NPE at line 294, do you know if it is because of
>>>> >>>>>>>>>>> > context being
>>>> >>>>>>>>>>> > null
>>>> >>>>>>>>>>> > or context.rules ?
>>>> >>>>>>>>>>>
>>>> >>>>>>>>>>> I don't know. I didn't look into it far enough. I supposed,
>>>> >>>>>>>>>>> though,
>>>> >>>>>>>>>>> if I'm the only one seeing this issue, that means I've got
>>>> >>>>>>>>>>> something
>>>> >>>>>>>>>>> wrong somewhere.
>>>> >>>>>>>>>>>
>>>> >>>>>>>>>>
>>>> >>>>>>>>>>
>>>> >>>>>>>>>
>>>> >>>>>>>>>
>>>> >>>>>>>>> ------------------------------------------------------------------------------
>>>> >>>>>>>>> Come build with us! The BlackBerry® Developer Conference in
>>>> >>>>>>>>> SF, CA
>>>> >>>>>>>>> is the only developer event you need to attend this year.
>>>> >>>>>>>>> Jumpstart your
>>>> >>>>>>>>> developing skills, take BlackBerry mobile applications to market
>>>> >>>>>>>>> and stay
>>>> >>>>>>>>> ahead of the curve. Join us from November 9-12, 2009.
>>>> >>>>>>>>> Register now!
>>>> >>>>>>>>> http://p.sf.net/sfu/devconf
>>>> >>>>>>>>> --
>>>> >>>>>>>>> -----------------------------------------------
>>>> >>>>>>>>> jEdit Developers' List
>>>> >>>>>>>>> jEd...@li...
>>>> >>>>>>>>> https://lists.sourceforge.net/lists/listinfo/jedit-devel
>>>> >>>>>>>>>
>>>> >>>>>>>>
>>>> >>>>>>>
>>>> >>>>>>
>>>> >>>>>
>>>> >>>>
>>>> >>>
>>>> >>
>>>> >
>>>> >
>>>> > ------------------------------------------------------------------------------
>>>> > Come build with us! The BlackBerry® Developer Conference in SF, CA
>>>> > is the only developer event you need to attend this year. Jumpstart your
>>>> > developing skills, take BlackBerry mobile applications to market and
>>>> > stay
>>>> > ahead of the curve. Join us from November 9-12, 2009. Register
>>>> > now!
>>>> > http://p.sf.net/sfu/devconf
>>>> > --
>>>> > -----------------------------------------------
>>>> > jEdit Developers' List
>>>> > jEd...@li...
>>>> > https://lists.sourceforge.net/lists/listinfo/jedit-devel
>>>> >
>>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>> Come build with us! The BlackBerry® Developer Conference in SF, CA
>>>> is the only developer event you need to attend this year. Jumpstart your
>>>> developing skills, take BlackBerry mobile applications to market and stay
>>>> ahead of the curve. Join us from November 9-12, 2009. Register
>>>> now!
>>>> http://p.sf.net/sfu/devconf
>>>> --
>>>> -----------------------------------------------
>>>> jEdit Developers' List
>>>> jEd...@li...
>>>> https://lists.sourceforge.net/lists/listinfo/jedit-devel
>>>
>>>
>>
>
|
|
From: Shlomy R. <sre...@gm...> - 2009-10-05 15:42:46
|
That's nice, but did you check the performance? Before replacing a
very simple fix like 'synchronized' with such a large fix (even if
trivial), do you have some performance benchmarking results that show
that your fix improves performance?
The performance cost of creating a new data object and copying the
values into its members might be even larger than synchronizing the
method...
Shlomy
On Mon, Oct 5, 2009 at 5:34 PM, Dale Anson <da...@gr...> wrote:
> Please see attached. I attached the whole file rather than a patch
> since the patch turned out to be quite large. There are a lot of
> small changes. Essentially, I turned the instance fields of
> TokenMarker into a data object, and am passing that data object
> around. Each call to markTokens gets its own data object, which keeps
> one call from interfering with the values being used from another
> call. I've been looking into how to write a unit test markTokens, and
> would like to get that done before committing a change like this.
>
> Dale
>
>
> On Sun, Oct 4, 2009 at 7:42 AM, Dale Anson <da...@gr...> wrote:
>> Making markTokens re-entrant would help performance in TaskList also.
>> TaskList uses a synchronized method to find tasks, the method is
>> synchronized to get around threading issues. I think if markTokens
>> was re-entrant, this TaskList method would not need to be
>> synchronized.
>>
>> It is more work to make markTokens re-entrant, but it looks to be
>> straight-forward to do. I'll give it a shot today and pass on the
>> results for review.
>>
>> Dale
>>
>>
>> On Sun, Oct 4, 2009 at 7:13 AM, Shlomy Reinstein <sre...@gm...> wrote:
>>> I agree that making TokenMarker re-entrant rather than synchronized is
>>> better for performance. But like I wrote before, there are several
>>> fields that are modified by markTokens and its callees, and making it
>>> re-entrant is a larger change which might introduce another bug. If
>>> you'd like to do this yourself, feel free to...
>>>
>>> Unless you really can show a slowness in jEdit as a result of this,
>>> let's stay with the simple and safe fix. It fixes a real issue and I
>>> don't see it causing any.
>>>
>>> Shlomy
>>>
>>> On Sun, Oct 4, 2009 at 12:05 PM, Matthieu Casanova
>>> <cho...@gm...> wrote:
>>>> Hi in fact instead of making TokenMarker synchronized why not using method
>>>> parameters instead of fields so the TokenMarker would be threadsafe without
>>>> using synchronized code which is slow and since this code is an important
>>>> point for performances I think it should be as fast as possible
>>>> Matthieu
>>>>
>>>> 2009/10/3 Shlomy Reinstein <sre...@gm...>
>>>>>
>>>>> It would (if possible), but that would require a larger change, which
>>>>> has a potential of introducing another bug. The one I did just adds
>>>>> the word "synchronized"... If there is a problem with that, I will
>>>>> consider another change. I am not completely sure it can be made
>>>>> reentrant, though.
>>>>>
>>>>> Shlomy
>>>>>
>>>>> On Sat, Oct 3, 2009 at 5:13 PM, Dale Anson <da...@gr...> wrote:
>>>>> > woops, just saw your other email about TaskList.
>>>>> >
>>>>> > Wouldn't it be better to make TokenMarker reentrant rather than
>>>>> > synchronized?
>>>>> >
>>>>> > Dale
>>>>> >
>>>>> >
>>>>> > On Sat, Oct 3, 2009 at 9:08 AM, Dale Anson <da...@gr...> wrote:
>>>>> >> What was the issue with TaskList? And which TaskList -- the one in
>>>>> >> PluginManager or the one from Eric's daily builds?
>>>>> >>
>>>>> >> Dale
>>>>> >>
>>>>> >>
>>>>> >> On Sat, Oct 3, 2009 at 7:39 AM, Shlomy Reinstein <sre...@gm...>
>>>>> >> wrote:
>>>>> >>> I have committed this small change (make TokenMarker.markTokens
>>>>> >>> synchronized) and it seems to solve the issue with TaskList.
>>>>> >>> I also haven't noticed any performance impact of this. If anyone
>>>>> >>> discovers a performance problem caused by this, let me know and I'll
>>>>> >>> fix it.
>>>>> >>>
>>>>> >>> Shlomy
>>>>> >>>
>>>>> >>> On Sat, Oct 3, 2009 at 11:40 AM, Shlomy Reinstein <sre...@gm...>
>>>>> >>> wrote:
>>>>> >>>> Hi,
>>>>> >>>>
>>>>> >>>> It turns out that TokenMarker.markTokens is not thread-safe. It needs
>>>>> >>>> to be synchronized. It has a 'context' field, that is potentially
>>>>> >>>> updated each time "markTokens" is called. If called simultaneously by
>>>>> >>>> multiple threads, each may modify it, causing the other threads to
>>>>> >>>> use
>>>>> >>>> an incorrect context.
>>>>> >>>>
>>>>> >>>> I just verified that the problem happens very frequently with the
>>>>> >>>> TaskList plugin. TaskList has a dockable which shows the tasks in the
>>>>> >>>> current buffer, and also highlights tasks in the text area. To find
>>>>> >>>> the tasks in the buffer, it uses "markTokens" in the background. So,
>>>>> >>>> when this plugin is in use, I experience all kinds of painting issues
>>>>> >>>> and occasionally the NPE that Dale experienced.
>>>>> >>>> Disabling the TaskList plugin gets rid of all these problems.
>>>>> >>>>
>>>>> >>>> However, it is not a TaskList problem, "markTokens" simply needs to
>>>>> >>>> be
>>>>> >>>> synchronized. If this causes some performance problem in the core,
>>>>> >>>> then we should have two separate paths to markTokens, one for the
>>>>> >>>> core
>>>>> >>>> and one for plugins, and make the plugin's path ensure thread-safety,
>>>>> >>>> e.g. by deferring the task to the AWT thread.
>>>>> >>>>
>>>>> >>>> Shlomy
>>>>> >>>>
>>>>> >>>> On Sat, Oct 3, 2009 at 3:39 AM, Dale Anson <da...@gr...>
>>>>> >>>> wrote:
>>>>> >>>>> Actually, I verified that context.parent is occasionally null, which
>>>>> >>>>> causes the NPE when its 'rules' field is referenced. Thinking about
>>>>> >>>>> this some more, I bet is it's a threading issue. It could be that
>>>>> >>>>> in
>>>>> >>>>> the first check (in the code you sent) it is not null, but is reset
>>>>> >>>>> to
>>>>> >>>>> null before the method is actually invoked. That would mean my
>>>>> >>>>> patch
>>>>> >>>>> is not very good since it doesn't address thread-safety.
>>>>> >>>>>
>>>>> >>>>> I did double check, I've got source for jEdit at revision 16233.
>>>>> >>>>> The
>>>>> >>>>> only change I have is the one I made to this class. I verified that
>>>>> >>>>> I'm using the standard java mode from svn also, and it's while
>>>>> >>>>> editing
>>>>> >>>>> java files that I know I've seen the problem. I'm not sure if I've
>>>>> >>>>> seen this issue in other modes.
>>>>> >>>>>
>>>>> >>>>> Dale
>>>>> >>>>>
>>>>> >>>>>
>>>>> >>>>> On Fri, Oct 2, 2009 at 2:46 AM, Shlomy Reinstein
>>>>> >>>>> <sre...@gm...> wrote:
>>>>> >>>>>> I took a more thorough look at the code. It is absolutely
>>>>> >>>>>> IMPOSSIBLE
>>>>> >>>>>> that context.parent is null in the method that generates the NPE
>>>>> >>>>>> for
>>>>> >>>>>> you, unless you've changed something in the jedit core. There is
>>>>> >>>>>> only
>>>>> >>>>>> one place where the method generating the NPE is called:
>>>>> >>>>>> //{{{ check for end of delegate
>>>>> >>>>>> if (context.parent != null
>>>>> >>>>>> && context.parent.inRule != null
>>>>> >>>>>> &&
>>>>> >>>>>> checkDelegateEnd(context.parent.inRule))
>>>>> >>>>>> {
>>>>> >>>>>> seenWhitespaceEnd = true;
>>>>> >>>>>> continue main_loop;
>>>>> >>>>>> } //}}}
>>>>> >>>>>>
>>>>> >>>>>> Note the check for "context.parent" not being null before calling
>>>>> >>>>>> this method.
>>>>> >>>>>> An NPE can still be caused by this line, but it won't be for
>>>>> >>>>>> "context.parent" being null, rather it would be for
>>>>> >>>>>> "context.parent.rules" being null.
>>>>> >>>>>>
>>>>> >>>>>> Shlomy
>>>>> >>>>>>
>>>>> >>>>>> On Fri, Oct 2, 2009 at 8:15 AM, Dale Anson <da...@gr...>
>>>>> >>>>>> wrote:
>>>>> >>>>>>> No, I can't reproduce it reliably. Somehow, I get into this state
>>>>> >>>>>>> after a couple of hours of running jEdit. Usually, it happens
>>>>> >>>>>>> when
>>>>> >>>>>>> I'm editing somewhere in the middle of a file, I'll hit Enter at
>>>>> >>>>>>> the
>>>>> >>>>>>> end of a line to start the next line and then there is the
>>>>> >>>>>>> trouble. I
>>>>> >>>>>>> have verified that it is definitely the case that context.parent
>>>>> >>>>>>> is
>>>>> >>>>>>> sometimes null. I see in LineContext itself that there are a
>>>>> >>>>>>> several
>>>>> >>>>>>> checks for a null parent, but not elsewhere. I think I've only
>>>>> >>>>>>> seen
>>>>> >>>>>>> this with java files, but I'm not absolutely certain.
>>>>> >>>>>>>
>>>>> >>>>>>> Do you know of a better thing to do if context.parent is null?
>>>>> >>>>>>>
>>>>> >>>>>>> Dale
>>>>> >>>>>>>
>>>>> >>>>>>>
>>>>> >>>>>>> On Thu, Oct 1, 2009 at 11:31 PM, Shlomy Reinstein
>>>>> >>>>>>> <sre...@gm...> wrote:
>>>>> >>>>>>>> Hi,
>>>>> >>>>>>>> Can you reproduce this every time with some example?
>>>>> >>>>>>>> If not , does this happen when you are editing (i.e. pressing
>>>>> >>>>>>>> enter
>>>>> >>>>>>>> on) the first or second line, or also when editing subsequent
>>>>> >>>>>>>> lines
>>>>> >>>>>>>> down the file?
>>>>> >>>>>>>> The 'parent' is usually the LineContext of the previous line.
>>>>> >>>>>>>> Shlomy
>>>>> >>>>>>>>
>>>>> >>>>>>>> On Fri, Oct 2, 2009 at 3:33 AM, Dale Anson <da...@gr...>
>>>>> >>>>>>>> wrote:
>>>>> >>>>>>>>> I entered a patch to fix this, see tracker 2871624. I did some
>>>>> >>>>>>>>> more
>>>>> >>>>>>>>> investigation, and it turns out that context.parent can be null.
>>>>> >>>>>>>>> The
>>>>> >>>>>>>>> patch checks for this case, and if the parent is null, it just
>>>>> >>>>>>>>> continues to use the same context. So far, this seems to be
>>>>> >>>>>>>>> working
>>>>> >>>>>>>>> fine.
>>>>> >>>>>>>>>
>>>>> >>>>>>>>> Dale
>>>>> >>>>>>>>>
>>>>> >>>>>>>>>
>>>>> >>>>>>>>> On Wed, Sep 30, 2009 at 1:07 PM, Alan Ezust
>>>>> >>>>>>>>> <ala...@gm...> wrote:
>>>>> >>>>>>>>>> I've seen an NPE there from time to time also, but I can't
>>>>> >>>>>>>>>> reproduce it.
>>>>> >>>>>>>>>>
>>>>> >>>>>>>>>> On Wed, Sep 30, 2009 at 11:50 AM, Dale Anson
>>>>> >>>>>>>>>> <da...@gr...> wrote:
>>>>> >>>>>>>>>>>
>>>>> >>>>>>>>>>> On Wed, Sep 30, 2009 at 9:58 AM, Matthieu Casanova
>>>>> >>>>>>>>>>> <cho...@gm...> wrote:
>>>>> >>>>>>>>>>> > But in that code, the fields are used as parameters, so
>>>>> >>>>>>>>>>> > filling the
>>>>> >>>>>>>>>>> > keyword
>>>>> >>>>>>>>>>> > field at line 294 is used to highlight tokens, removing that
>>>>> >>>>>>>>>>> > will lead
>>>>> >>>>>>>>>>> > to
>>>>> >>>>>>>>>>> > bugs.
>>>>> >>>>>>>>>>> > For the NPE at line 294, do you know if it is because of
>>>>> >>>>>>>>>>> > context being
>>>>> >>>>>>>>>>> > null
>>>>> >>>>>>>>>>> > or context.rules ?
>>>>> >>>>>>>>>>>
>>>>> >>>>>>>>>>> I don't know. I didn't look into it far enough. I supposed,
>>>>> >>>>>>>>>>> though,
>>>>> >>>>>>>>>>> if I'm the only one seeing this issue, that means I've got
>>>>> >>>>>>>>>>> something
>>>>> >>>>>>>>>>> wrong somewhere.
>>>>> >>>>>>>>>>>
>>>>> >>>>>>>>>>
>>>>> >>>>>>>>>>
>>>>> >>>>>>>>>
>>>>> >>>>>>>>>
>>>>> >>>>>>>>> ------------------------------------------------------------------------------
>>>>> >>>>>>>>> Come build with us! The BlackBerry® Developer Conference in
>>>>> >>>>>>>>> SF, CA
>>>>> >>>>>>>>> is the only developer event you need to attend this year.
>>>>> >>>>>>>>> Jumpstart your
>>>>> >>>>>>>>> developing skills, take BlackBerry mobile applications to market
>>>>> >>>>>>>>> and stay
>>>>> >>>>>>>>> ahead of the curve. Join us from November 9-12, 2009.
>>>>> >>>>>>>>> Register now!
>>>>> >>>>>>>>> http://p.sf.net/sfu/devconf
>>>>> >>>>>>>>> --
>>>>> >>>>>>>>> -----------------------------------------------
>>>>> >>>>>>>>> jEdit Developers' List
>>>>> >>>>>>>>> jEd...@li...
>>>>> >>>>>>>>> https://lists.sourceforge.net/lists/listinfo/jedit-devel
>>>>> >>>>>>>>>
>>>>> >>>>>>>>
>>>>> >>>>>>>
>>>>> >>>>>>
>>>>> >>>>>
>>>>> >>>>
>>>>> >>>
>>>>> >>
>>>>> >
>>>>> >
>>>>> > ------------------------------------------------------------------------------
>>>>> > Come build with us! The BlackBerry® Developer Conference in SF, CA
>>>>> > is the only developer event you need to attend this year. Jumpstart your
>>>>> > developing skills, take BlackBerry mobile applications to market and
>>>>> > stay
>>>>> > ahead of the curve. Join us from November 9-12, 2009. Register
>>>>> > now!
>>>>> > http://p.sf.net/sfu/devconf
>>>>> > --
>>>>> > -----------------------------------------------
>>>>> > jEdit Developers' List
>>>>> > jEd...@li...
>>>>> > https://lists.sourceforge.net/lists/listinfo/jedit-devel
>>>>> >
>>>>>
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>> Come build with us! The BlackBerry® Developer Conference in SF, CA
>>>>> is the only developer event you need to attend this year. Jumpstart your
>>>>> developing skills, take BlackBerry mobile applications to market and stay
>>>>> ahead of the curve. Join us from November 9-12, 2009. Register
>>>>> now!
>>>>> http://p.sf.net/sfu/devconf
>>>>> --
>>>>> -----------------------------------------------
>>>>> jEdit Developers' List
>>>>> jEd...@li...
>>>>> https://lists.sourceforge.net/lists/listinfo/jedit-devel
>>>>
>>>>
>>>
>>
>
> ------------------------------------------------------------------------------
> Come build with us! The BlackBerry® Developer Conference in SF, CA
> is the only developer event you need to attend this year. Jumpstart your
> developing skills, take BlackBerry mobile applications to market and stay
> ahead of the curve. Join us from November 9-12, 2009. Register now!
> http://p.sf.net/sfu/devconf
> --
> -----------------------------------------------
> jEdit Developers' List
> jEd...@li...
> https://lists.sourceforge.net/lists/listinfo/jedit-devel
>
>
|
|
From: Matthieu C. <cho...@gm...> - 2009-10-05 15:47:23
|
Hi, that's what I thought about but you were quick.
Another change could be to have an instance of TokenMarkerData that would be
used if the caller thread is AWTThread, and if not create a new instance.
Since it will be AWTThread that is used most of the time it would save work
for the garbage collector, don't you think ?
Matthieu
2009/10/5 Dale Anson <da...@gr...>
> Please see attached. I attached the whole file rather than a patch
> since the patch turned out to be quite large. There are a lot of
> small changes. Essentially, I turned the instance fields of
> TokenMarker into a data object, and am passing that data object
> around. Each call to markTokens gets its own data object, which keeps
> one call from interfering with the values being used from another
> call. I've been looking into how to write a unit test markTokens, and
> would like to get that done before committing a change like this.
>
> Dale
>
>
> On Sun, Oct 4, 2009 at 7:42 AM, Dale Anson <da...@gr...> wrote:
> > Making markTokens re-entrant would help performance in TaskList also.
> > TaskList uses a synchronized method to find tasks, the method is
> > synchronized to get around threading issues. I think if markTokens
> > was re-entrant, this TaskList method would not need to be
> > synchronized.
> >
> > It is more work to make markTokens re-entrant, but it looks to be
> > straight-forward to do. I'll give it a shot today and pass on the
> > results for review.
> >
> > Dale
> >
> >
> > On Sun, Oct 4, 2009 at 7:13 AM, Shlomy Reinstein <sre...@gm...>
> wrote:
> >> I agree that making TokenMarker re-entrant rather than synchronized is
> >> better for performance. But like I wrote before, there are several
> >> fields that are modified by markTokens and its callees, and making it
> >> re-entrant is a larger change which might introduce another bug. If
> >> you'd like to do this yourself, feel free to...
> >>
> >> Unless you really can show a slowness in jEdit as a result of this,
> >> let's stay with the simple and safe fix. It fixes a real issue and I
> >> don't see it causing any.
> >>
> >> Shlomy
> >>
> >> On Sun, Oct 4, 2009 at 12:05 PM, Matthieu Casanova
> >> <cho...@gm...> wrote:
> >>> Hi in fact instead of making TokenMarker synchronized why not using
> method
> >>> parameters instead of fields so the TokenMarker would be threadsafe
> without
> >>> using synchronized code which is slow and since this code is an
> important
> >>> point for performances I think it should be as fast as possible
> >>> Matthieu
> >>>
> >>> 2009/10/3 Shlomy Reinstein <sre...@gm...>
> >>>>
> >>>> It would (if possible), but that would require a larger change, which
> >>>> has a potential of introducing another bug. The one I did just adds
> >>>> the word "synchronized"... If there is a problem with that, I will
> >>>> consider another change. I am not completely sure it can be made
> >>>> reentrant, though.
> >>>>
> >>>> Shlomy
> >>>>
> >>>> On Sat, Oct 3, 2009 at 5:13 PM, Dale Anson <da...@gr...>
> wrote:
> >>>> > woops, just saw your other email about TaskList.
> >>>> >
> >>>> > Wouldn't it be better to make TokenMarker reentrant rather than
> >>>> > synchronized?
> >>>> >
> >>>> > Dale
> >>>> >
> >>>> >
> >>>> > On Sat, Oct 3, 2009 at 9:08 AM, Dale Anson <da...@gr...>
> wrote:
> >>>> >> What was the issue with TaskList? And which TaskList -- the one in
> >>>> >> PluginManager or the one from Eric's daily builds?
> >>>> >>
> >>>> >> Dale
> >>>> >>
> >>>> >>
> >>>> >> On Sat, Oct 3, 2009 at 7:39 AM, Shlomy Reinstein <
> sre...@gm...>
> >>>> >> wrote:
> >>>> >>> I have committed this small change (make TokenMarker.markTokens
> >>>> >>> synchronized) and it seems to solve the issue with TaskList.
> >>>> >>> I also haven't noticed any performance impact of this. If anyone
> >>>> >>> discovers a performance problem caused by this, let me know and
> I'll
> >>>> >>> fix it.
> >>>> >>>
> >>>> >>> Shlomy
> >>>> >>>
> >>>> >>> On Sat, Oct 3, 2009 at 11:40 AM, Shlomy Reinstein <
> sre...@gm...>
> >>>> >>> wrote:
> >>>> >>>> Hi,
> >>>> >>>>
> >>>> >>>> It turns out that TokenMarker.markTokens is not thread-safe. It
> needs
> >>>> >>>> to be synchronized. It has a 'context' field, that is potentially
> >>>> >>>> updated each time "markTokens" is called. If called
> simultaneously by
> >>>> >>>> multiple threads, each may modify it, causing the other threads
> to
> >>>> >>>> use
> >>>> >>>> an incorrect context.
> >>>> >>>>
> >>>> >>>> I just verified that the problem happens very frequently with the
> >>>> >>>> TaskList plugin. TaskList has a dockable which shows the tasks in
> the
> >>>> >>>> current buffer, and also highlights tasks in the text area. To
> find
> >>>> >>>> the tasks in the buffer, it uses "markTokens" in the background.
> So,
> >>>> >>>> when this plugin is in use, I experience all kinds of painting
> issues
> >>>> >>>> and occasionally the NPE that Dale experienced.
> >>>> >>>> Disabling the TaskList plugin gets rid of all these problems.
> >>>> >>>>
> >>>> >>>> However, it is not a TaskList problem, "markTokens" simply needs
> to
> >>>> >>>> be
> >>>> >>>> synchronized. If this causes some performance problem in the
> core,
> >>>> >>>> then we should have two separate paths to markTokens, one for the
> >>>> >>>> core
> >>>> >>>> and one for plugins, and make the plugin's path ensure
> thread-safety,
> >>>> >>>> e.g. by deferring the task to the AWT thread.
> >>>> >>>>
> >>>> >>>> Shlomy
> >>>> >>>>
> >>>> >>>> On Sat, Oct 3, 2009 at 3:39 AM, Dale Anson <da...@gr...>
> >>>> >>>> wrote:
> >>>> >>>>> Actually, I verified that context.parent is occasionally null,
> which
> >>>> >>>>> causes the NPE when its 'rules' field is referenced. Thinking
> about
> >>>> >>>>> this some more, I bet is it's a threading issue. It could be
> that
> >>>> >>>>> in
> >>>> >>>>> the first check (in the code you sent) it is not null, but is
> reset
> >>>> >>>>> to
> >>>> >>>>> null before the method is actually invoked. That would mean my
> >>>> >>>>> patch
> >>>> >>>>> is not very good since it doesn't address thread-safety.
> >>>> >>>>>
> >>>> >>>>> I did double check, I've got source for jEdit at revision 16233.
> >>>> >>>>> The
> >>>> >>>>> only change I have is the one I made to this class. I verified
> that
> >>>> >>>>> I'm using the standard java mode from svn also, and it's while
> >>>> >>>>> editing
> >>>> >>>>> java files that I know I've seen the problem. I'm not sure if
> I've
> >>>> >>>>> seen this issue in other modes.
> >>>> >>>>>
> >>>> >>>>> Dale
> >>>> >>>>>
> >>>> >>>>>
> >>>> >>>>> On Fri, Oct 2, 2009 at 2:46 AM, Shlomy Reinstein
> >>>> >>>>> <sre...@gm...> wrote:
> >>>> >>>>>> I took a more thorough look at the code. It is absolutely
> >>>> >>>>>> IMPOSSIBLE
> >>>> >>>>>> that context.parent is null in the method that generates the
> NPE
> >>>> >>>>>> for
> >>>> >>>>>> you, unless you've changed something in the jedit core. There
> is
> >>>> >>>>>> only
> >>>> >>>>>> one place where the method generating the NPE is called:
> >>>> >>>>>> //{{{ check for end of delegate
> >>>> >>>>>> if (context.parent != null
> >>>> >>>>>> && context.parent.inRule != null
> >>>> >>>>>> &&
> >>>> >>>>>> checkDelegateEnd(context.parent.inRule))
> >>>> >>>>>> {
> >>>> >>>>>> seenWhitespaceEnd = true;
> >>>> >>>>>> continue main_loop;
> >>>> >>>>>> } //}}}
> >>>> >>>>>>
> >>>> >>>>>> Note the check for "context.parent" not being null before
> calling
> >>>> >>>>>> this method.
> >>>> >>>>>> An NPE can still be caused by this line, but it won't be for
> >>>> >>>>>> "context.parent" being null, rather it would be for
> >>>> >>>>>> "context.parent.rules" being null.
> >>>> >>>>>>
> >>>> >>>>>> Shlomy
> >>>> >>>>>>
> >>>> >>>>>> On Fri, Oct 2, 2009 at 8:15 AM, Dale Anson <
> da...@gr...>
> >>>> >>>>>> wrote:
> >>>> >>>>>>> No, I can't reproduce it reliably. Somehow, I get into this
> state
> >>>> >>>>>>> after a couple of hours of running jEdit. Usually, it happens
> >>>> >>>>>>> when
> >>>> >>>>>>> I'm editing somewhere in the middle of a file, I'll hit Enter
> at
> >>>> >>>>>>> the
> >>>> >>>>>>> end of a line to start the next line and then there is the
> >>>> >>>>>>> trouble. I
> >>>> >>>>>>> have verified that it is definitely the case that
> context.parent
> >>>> >>>>>>> is
> >>>> >>>>>>> sometimes null. I see in LineContext itself that there are a
> >>>> >>>>>>> several
> >>>> >>>>>>> checks for a null parent, but not elsewhere. I think I've
> only
> >>>> >>>>>>> seen
> >>>> >>>>>>> this with java files, but I'm not absolutely certain.
> >>>> >>>>>>>
> >>>> >>>>>>> Do you know of a better thing to do if context.parent is null?
> >>>> >>>>>>>
> >>>> >>>>>>> Dale
> >>>> >>>>>>>
> >>>> >>>>>>>
> >>>> >>>>>>> On Thu, Oct 1, 2009 at 11:31 PM, Shlomy Reinstein
> >>>> >>>>>>> <sre...@gm...> wrote:
> >>>> >>>>>>>> Hi,
> >>>> >>>>>>>> Can you reproduce this every time with some example?
> >>>> >>>>>>>> If not , does this happen when you are editing (i.e. pressing
> >>>> >>>>>>>> enter
> >>>> >>>>>>>> on) the first or second line, or also when editing subsequent
> >>>> >>>>>>>> lines
> >>>> >>>>>>>> down the file?
> >>>> >>>>>>>> The 'parent' is usually the LineContext of the previous line.
> >>>> >>>>>>>> Shlomy
> >>>> >>>>>>>>
> >>>> >>>>>>>> On Fri, Oct 2, 2009 at 3:33 AM, Dale Anson <
> da...@gr...>
> >>>> >>>>>>>> wrote:
> >>>> >>>>>>>>> I entered a patch to fix this, see tracker 2871624. I did
> some
> >>>> >>>>>>>>> more
> >>>> >>>>>>>>> investigation, and it turns out that context.parent can be
> null.
> >>>> >>>>>>>>> The
> >>>> >>>>>>>>> patch checks for this case, and if the parent is null, it
> just
> >>>> >>>>>>>>> continues to use the same context. So far, this seems to be
> >>>> >>>>>>>>> working
> >>>> >>>>>>>>> fine.
> >>>> >>>>>>>>>
> >>>> >>>>>>>>> Dale
> >>>> >>>>>>>>>
> >>>> >>>>>>>>>
> >>>> >>>>>>>>> On Wed, Sep 30, 2009 at 1:07 PM, Alan Ezust
> >>>> >>>>>>>>> <ala...@gm...> wrote:
> >>>> >>>>>>>>>> I've seen an NPE there from time to time also, but I can't
> >>>> >>>>>>>>>> reproduce it.
> >>>> >>>>>>>>>>
> >>>> >>>>>>>>>> On Wed, Sep 30, 2009 at 11:50 AM, Dale Anson
> >>>> >>>>>>>>>> <da...@gr...> wrote:
> >>>> >>>>>>>>>>>
> >>>> >>>>>>>>>>> On Wed, Sep 30, 2009 at 9:58 AM, Matthieu Casanova
> >>>> >>>>>>>>>>> <cho...@gm...> wrote:
> >>>> >>>>>>>>>>> > But in that code, the fields are used as parameters, so
> >>>> >>>>>>>>>>> > filling the
> >>>> >>>>>>>>>>> > keyword
> >>>> >>>>>>>>>>> > field at line 294 is used to highlight tokens, removing
> that
> >>>> >>>>>>>>>>> > will lead
> >>>> >>>>>>>>>>> > to
> >>>> >>>>>>>>>>> > bugs.
> >>>> >>>>>>>>>>> > For the NPE at line 294, do you know if it is because of
> >>>> >>>>>>>>>>> > context being
> >>>> >>>>>>>>>>> > null
> >>>> >>>>>>>>>>> > or context.rules ?
> >>>> >>>>>>>>>>>
> >>>> >>>>>>>>>>> I don't know. I didn't look into it far enough. I
> supposed,
> >>>> >>>>>>>>>>> though,
> >>>> >>>>>>>>>>> if I'm the only one seeing this issue, that means I've got
> >>>> >>>>>>>>>>> something
> >>>> >>>>>>>>>>> wrong somewhere.
> >>>> >>>>>>>>>>>
> >>>> >>>>>>>>>>
> >>>> >>>>>>>>>>
> >>>> >>>>>>>>>
> >>>> >>>>>>>>>
> >>>> >>>>>>>>>
> ------------------------------------------------------------------------------
> >>>> >>>>>>>>> Come build with us! The BlackBerry® Developer Conference
> in
> >>>> >>>>>>>>> SF, CA
> >>>> >>>>>>>>> is the only developer event you need to attend this year.
> >>>> >>>>>>>>> Jumpstart your
> >>>> >>>>>>>>> developing skills, take BlackBerry mobile applications to
> market
> >>>> >>>>>>>>> and stay
> >>>> >>>>>>>>> ahead of the curve. Join us from November 9-12, 2009.
> >>>> >>>>>>>>> Register now!
> >>>> >>>>>>>>> http://p.sf.net/sfu/devconf
> >>>> >>>>>>>>> --
> >>>> >>>>>>>>> -----------------------------------------------
> >>>> >>>>>>>>> jEdit Developers' List
> >>>> >>>>>>>>> jEd...@li...
> >>>> >>>>>>>>> https://lists.sourceforge.net/lists/listinfo/jedit-devel
> >>>> >>>>>>>>>
> >>>> >>>>>>>>
> >>>> >>>>>>>
> >>>> >>>>>>
> >>>> >>>>>
> >>>> >>>>
> >>>> >>>
> >>>> >>
> >>>> >
> >>>> >
> >>>> >
> ------------------------------------------------------------------------------
> >>>> > Come build with us! The BlackBerry® Developer Conference in SF,
> CA
> >>>> > is the only developer event you need to attend this year. Jumpstart
> your
> >>>> > developing skills, take BlackBerry mobile applications to market and
> >>>> > stay
> >>>> > ahead of the curve. Join us from November 9-12, 2009. Register
> >>>> > now!
> >>>> > http://p.sf.net/sfu/devconf
> >>>> > --
> >>>> > -----------------------------------------------
> >>>> > jEdit Developers' List
> >>>> > jEd...@li...
> >>>> > https://lists.sourceforge.net/lists/listinfo/jedit-devel
> >>>> >
> >>>>
> >>>>
> >>>>
> ------------------------------------------------------------------------------
> >>>> Come build with us! The BlackBerry® Developer Conference in SF, CA
> >>>> is the only developer event you need to attend this year. Jumpstart
> your
> >>>> developing skills, take BlackBerry mobile applications to market and
> stay
> >>>> ahead of the curve. Join us from November 9-12, 2009. Register
> >>>> now!
> >>>> http://p.sf.net/sfu/devconf
> >>>> --
> >>>> -----------------------------------------------
> >>>> jEdit Developers' List
> >>>> jEd...@li...
> >>>> https://lists.sourceforge.net/lists/listinfo/jedit-devel
> >>>
> >>>
> >>
> >
>
>
> ------------------------------------------------------------------------------
> Come build with us! The BlackBerry® Developer Conference in SF, CA
> is the only developer event you need to attend this year. Jumpstart your
> developing skills, take BlackBerry mobile applications to market and stay
> ahead of the curve. Join us from November 9-12, 2009. Register now!
> http://p.sf.net/sfu/devconf
> --
> -----------------------------------------------
> jEdit Developers' List
> jEd...@li...
> https://lists.sourceforge.net/lists/listinfo/jedit-devel
>
>
|
|
From: Shlomy R. <sre...@gm...> - 2009-10-05 15:55:31
|
Coming to think of it, I think that 'synchronized' is much better than
the larger change. Why? Because typically, both the core and the
plugins will call markTokens for the same line. So, the returned value
of TokenMarker.markTokens will be cached in the line manager, and the
2nd of them that comes to ask for the tokens will get the cached data
rather than re-computing it.
If you don't make it synchronized, both will compute it at the same
time, eventually one will override the other when the returned
LineContext is saved in the line manager.
Shlomy
On Mon, Oct 5, 2009 at 5:47 PM, Matthieu Casanova
<cho...@gm...> wrote:
> Hi, that's what I thought about but you were quick.
> Another change could be to have an instance of TokenMarkerData that would be
> used if the caller thread is AWTThread, and if not create a new instance.
> Since it will be AWTThread that is used most of the time it would save work
> for the garbage collector, don't you think ?
>
> Matthieu
>
> 2009/10/5 Dale Anson <da...@gr...>
>>
>> Please see attached. I attached the whole file rather than a patch
>> since the patch turned out to be quite large. There are a lot of
>> small changes. Essentially, I turned the instance fields of
>> TokenMarker into a data object, and am passing that data object
>> around. Each call to markTokens gets its own data object, which keeps
>> one call from interfering with the values being used from another
>> call. I've been looking into how to write a unit test markTokens, and
>> would like to get that done before committing a change like this.
>>
>> Dale
>>
>>
>> On Sun, Oct 4, 2009 at 7:42 AM, Dale Anson <da...@gr...> wrote:
>> > Making markTokens re-entrant would help performance in TaskList also.
>> > TaskList uses a synchronized method to find tasks, the method is
>> > synchronized to get around threading issues. I think if markTokens
>> > was re-entrant, this TaskList method would not need to be
>> > synchronized.
>> >
>> > It is more work to make markTokens re-entrant, but it looks to be
>> > straight-forward to do. I'll give it a shot today and pass on the
>> > results for review.
>> >
>> > Dale
>> >
>> >
>> > On Sun, Oct 4, 2009 at 7:13 AM, Shlomy Reinstein <sre...@gm...>
>> > wrote:
>> >> I agree that making TokenMarker re-entrant rather than synchronized is
>> >> better for performance. But like I wrote before, there are several
>> >> fields that are modified by markTokens and its callees, and making it
>> >> re-entrant is a larger change which might introduce another bug. If
>> >> you'd like to do this yourself, feel free to...
>> >>
>> >> Unless you really can show a slowness in jEdit as a result of this,
>> >> let's stay with the simple and safe fix. It fixes a real issue and I
>> >> don't see it causing any.
>> >>
>> >> Shlomy
>> >>
>> >> On Sun, Oct 4, 2009 at 12:05 PM, Matthieu Casanova
>> >> <cho...@gm...> wrote:
>> >>> Hi in fact instead of making TokenMarker synchronized why not using
>> >>> method
>> >>> parameters instead of fields so the TokenMarker would be threadsafe
>> >>> without
>> >>> using synchronized code which is slow and since this code is an
>> >>> important
>> >>> point for performances I think it should be as fast as possible
>> >>> Matthieu
>> >>>
>> >>> 2009/10/3 Shlomy Reinstein <sre...@gm...>
>> >>>>
>> >>>> It would (if possible), but that would require a larger change, which
>> >>>> has a potential of introducing another bug. The one I did just adds
>> >>>> the word "synchronized"... If there is a problem with that, I will
>> >>>> consider another change. I am not completely sure it can be made
>> >>>> reentrant, though.
>> >>>>
>> >>>> Shlomy
>> >>>>
>> >>>> On Sat, Oct 3, 2009 at 5:13 PM, Dale Anson <da...@gr...>
>> >>>> wrote:
>> >>>> > woops, just saw your other email about TaskList.
>> >>>> >
>> >>>> > Wouldn't it be better to make TokenMarker reentrant rather than
>> >>>> > synchronized?
>> >>>> >
>> >>>> > Dale
>> >>>> >
>> >>>> >
>> >>>> > On Sat, Oct 3, 2009 at 9:08 AM, Dale Anson <da...@gr...>
>> >>>> > wrote:
>> >>>> >> What was the issue with TaskList? And which TaskList -- the one
>> >>>> >> in
>> >>>> >> PluginManager or the one from Eric's daily builds?
>> >>>> >>
>> >>>> >> Dale
>> >>>> >>
>> >>>> >>
>> >>>> >> On Sat, Oct 3, 2009 at 7:39 AM, Shlomy Reinstein
>> >>>> >> <sre...@gm...>
>> >>>> >> wrote:
>> >>>> >>> I have committed this small change (make TokenMarker.markTokens
>> >>>> >>> synchronized) and it seems to solve the issue with TaskList.
>> >>>> >>> I also haven't noticed any performance impact of this. If anyone
>> >>>> >>> discovers a performance problem caused by this, let me know and
>> >>>> >>> I'll
>> >>>> >>> fix it.
>> >>>> >>>
>> >>>> >>> Shlomy
>> >>>> >>>
>> >>>> >>> On Sat, Oct 3, 2009 at 11:40 AM, Shlomy Reinstein
>> >>>> >>> <sre...@gm...>
>> >>>> >>> wrote:
>> >>>> >>>> Hi,
>> >>>> >>>>
>> >>>> >>>> It turns out that TokenMarker.markTokens is not thread-safe. It
>> >>>> >>>> needs
>> >>>> >>>> to be synchronized. It has a 'context' field, that is
>> >>>> >>>> potentially
>> >>>> >>>> updated each time "markTokens" is called. If called
>> >>>> >>>> simultaneously by
>> >>>> >>>> multiple threads, each may modify it, causing the other threads
>> >>>> >>>> to
>> >>>> >>>> use
>> >>>> >>>> an incorrect context.
>> >>>> >>>>
>> >>>> >>>> I just verified that the problem happens very frequently with
>> >>>> >>>> the
>> >>>> >>>> TaskList plugin. TaskList has a dockable which shows the tasks
>> >>>> >>>> in the
>> >>>> >>>> current buffer, and also highlights tasks in the text area. To
>> >>>> >>>> find
>> >>>> >>>> the tasks in the buffer, it uses "markTokens" in the background.
>> >>>> >>>> So,
>> >>>> >>>> when this plugin is in use, I experience all kinds of painting
>> >>>> >>>> issues
>> >>>> >>>> and occasionally the NPE that Dale experienced.
>> >>>> >>>> Disabling the TaskList plugin gets rid of all these problems.
>> >>>> >>>>
>> >>>> >>>> However, it is not a TaskList problem, "markTokens" simply needs
>> >>>> >>>> to
>> >>>> >>>> be
>> >>>> >>>> synchronized. If this causes some performance problem in the
>> >>>> >>>> core,
>> >>>> >>>> then we should have two separate paths to markTokens, one for
>> >>>> >>>> the
>> >>>> >>>> core
>> >>>> >>>> and one for plugins, and make the plugin's path ensure
>> >>>> >>>> thread-safety,
>> >>>> >>>> e.g. by deferring the task to the AWT thread.
>> >>>> >>>>
>> >>>> >>>> Shlomy
>> >>>> >>>>
>> >>>> >>>> On Sat, Oct 3, 2009 at 3:39 AM, Dale Anson <da...@gr...>
>> >>>> >>>> wrote:
>> >>>> >>>>> Actually, I verified that context.parent is occasionally null,
>> >>>> >>>>> which
>> >>>> >>>>> causes the NPE when its 'rules' field is referenced. Thinking
>> >>>> >>>>> about
>> >>>> >>>>> this some more, I bet is it's a threading issue. It could be
>> >>>> >>>>> that
>> >>>> >>>>> in
>> >>>> >>>>> the first check (in the code you sent) it is not null, but is
>> >>>> >>>>> reset
>> >>>> >>>>> to
>> >>>> >>>>> null before the method is actually invoked. That would mean my
>> >>>> >>>>> patch
>> >>>> >>>>> is not very good since it doesn't address thread-safety.
>> >>>> >>>>>
>> >>>> >>>>> I did double check, I've got source for jEdit at revision
>> >>>> >>>>> 16233.
>> >>>> >>>>> The
>> >>>> >>>>> only change I have is the one I made to this class. I verified
>> >>>> >>>>> that
>> >>>> >>>>> I'm using the standard java mode from svn also, and it's while
>> >>>> >>>>> editing
>> >>>> >>>>> java files that I know I've seen the problem. I'm not sure if
>> >>>> >>>>> I've
>> >>>> >>>>> seen this issue in other modes.
>> >>>> >>>>>
>> >>>> >>>>> Dale
>> >>>> >>>>>
>> >>>> >>>>>
>> >>>> >>>>> On Fri, Oct 2, 2009 at 2:46 AM, Shlomy Reinstein
>> >>>> >>>>> <sre...@gm...> wrote:
>> >>>> >>>>>> I took a more thorough look at the code. It is absolutely
>> >>>> >>>>>> IMPOSSIBLE
>> >>>> >>>>>> that context.parent is null in the method that generates the
>> >>>> >>>>>> NPE
>> >>>> >>>>>> for
>> >>>> >>>>>> you, unless you've changed something in the jedit core. There
>> >>>> >>>>>> is
>> >>>> >>>>>> only
>> >>>> >>>>>> one place where the method generating the NPE is called:
>> >>>> >>>>>> //{{{ check for end of delegate
>> >>>> >>>>>> if (context.parent != null
>> >>>> >>>>>> && context.parent.inRule != null
>> >>>> >>>>>> &&
>> >>>> >>>>>> checkDelegateEnd(context.parent.inRule))
>> >>>> >>>>>> {
>> >>>> >>>>>> seenWhitespaceEnd = true;
>> >>>> >>>>>> continue main_loop;
>> >>>> >>>>>> } //}}}
>> >>>> >>>>>>
>> >>>> >>>>>> Note the check for "context.parent" not being null before
>> >>>> >>>>>> calling
>> >>>> >>>>>> this method.
>> >>>> >>>>>> An NPE can still be caused by this line, but it won't be for
>> >>>> >>>>>> "context.parent" being null, rather it would be for
>> >>>> >>>>>> "context.parent.rules" being null.
>> >>>> >>>>>>
>> >>>> >>>>>> Shlomy
>> >>>> >>>>>>
>> >>>> >>>>>> On Fri, Oct 2, 2009 at 8:15 AM, Dale Anson
>> >>>> >>>>>> <da...@gr...>
>> >>>> >>>>>> wrote:
>> >>>> >>>>>>> No, I can't reproduce it reliably. Somehow, I get into this
>> >>>> >>>>>>> state
>> >>>> >>>>>>> after a couple of hours of running jEdit. Usually, it
>> >>>> >>>>>>> happens
>> >>>> >>>>>>> when
>> >>>> >>>>>>> I'm editing somewhere in the middle of a file, I'll hit Enter
>> >>>> >>>>>>> at
>> >>>> >>>>>>> the
>> >>>> >>>>>>> end of a line to start the next line and then there is the
>> >>>> >>>>>>> trouble. I
>> >>>> >>>>>>> have verified that it is definitely the case that
>> >>>> >>>>>>> context.parent
>> >>>> >>>>>>> is
>> >>>> >>>>>>> sometimes null. I see in LineContext itself that there are a
>> >>>> >>>>>>> several
>> >>>> >>>>>>> checks for a null parent, but not elsewhere. I think I've
>> >>>> >>>>>>> only
>> >>>> >>>>>>> seen
>> >>>> >>>>>>> this with java files, but I'm not absolutely certain.
>> >>>> >>>>>>>
>> >>>> >>>>>>> Do you know of a better thing to do if context.parent is
>> >>>> >>>>>>> null?
>> >>>> >>>>>>>
>> >>>> >>>>>>> Dale
>> >>>> >>>>>>>
>> >>>> >>>>>>>
>> >>>> >>>>>>> On Thu, Oct 1, 2009 at 11:31 PM, Shlomy Reinstein
>> >>>> >>>>>>> <sre...@gm...> wrote:
>> >>>> >>>>>>>> Hi,
>> >>>> >>>>>>>> Can you reproduce this every time with some example?
>> >>>> >>>>>>>> If not , does this happen when you are editing (i.e.
>> >>>> >>>>>>>> pressing
>> >>>> >>>>>>>> enter
>> >>>> >>>>>>>> on) the first or second line, or also when editing
>> >>>> >>>>>>>> subsequent
>> >>>> >>>>>>>> lines
>> >>>> >>>>>>>> down the file?
>> >>>> >>>>>>>> The 'parent' is usually the LineContext of the previous
>> >>>> >>>>>>>> line.
>> >>>> >>>>>>>> Shlomy
>> >>>> >>>>>>>>
>> >>>> >>>>>>>> On Fri, Oct 2, 2009 at 3:33 AM, Dale Anson
>> >>>> >>>>>>>> <da...@gr...>
>> >>>> >>>>>>>> wrote:
>> >>>> >>>>>>>>> I entered a patch to fix this, see tracker 2871624. I did
>> >>>> >>>>>>>>> some
>> >>>> >>>>>>>>> more
>> >>>> >>>>>>>>> investigation, and it turns out that context.parent can be
>> >>>> >>>>>>>>> null.
>> >>>> >>>>>>>>> The
>> >>>> >>>>>>>>> patch checks for this case, and if the parent is null, it
>> >>>> >>>>>>>>> just
>> >>>> >>>>>>>>> continues to use the same context. So far, this seems to
>> >>>> >>>>>>>>> be
>> >>>> >>>>>>>>> working
>> >>>> >>>>>>>>> fine.
>> >>>> >>>>>>>>>
>> >>>> >>>>>>>>> Dale
>> >>>> >>>>>>>>>
>> >>>> >>>>>>>>>
>> >>>> >>>>>>>>> On Wed, Sep 30, 2009 at 1:07 PM, Alan Ezust
>> >>>> >>>>>>>>> <ala...@gm...> wrote:
>> >>>> >>>>>>>>>> I've seen an NPE there from time to time also, but I can't
>> >>>> >>>>>>>>>> reproduce it.
>> >>>> >>>>>>>>>>
>> >>>> >>>>>>>>>> On Wed, Sep 30, 2009 at 11:50 AM, Dale Anson
>> >>>> >>>>>>>>>> <da...@gr...> wrote:
>> >>>> >>>>>>>>>>>
>> >>>> >>>>>>>>>>> On Wed, Sep 30, 2009 at 9:58 AM, Matthieu Casanova
>> >>>> >>>>>>>>>>> <cho...@gm...> wrote:
>> >>>> >>>>>>>>>>> > But in that code, the fields are used as parameters, so
>> >>>> >>>>>>>>>>> > filling the
>> >>>> >>>>>>>>>>> > keyword
>> >>>> >>>>>>>>>>> > field at line 294 is used to highlight tokens, removing
>> >>>> >>>>>>>>>>> > that
>> >>>> >>>>>>>>>>> > will lead
>> >>>> >>>>>>>>>>> > to
>> >>>> >>>>>>>>>>> > bugs.
>> >>>> >>>>>>>>>>> > For the NPE at line 294, do you know if it is because
>> >>>> >>>>>>>>>>> > of
>> >>>> >>>>>>>>>>> > context being
>> >>>> >>>>>>>>>>> > null
>> >>>> >>>>>>>>>>> > or context.rules ?
>> >>>> >>>>>>>>>>>
>> >>>> >>>>>>>>>>> I don't know. I didn't look into it far enough. I
>> >>>> >>>>>>>>>>> supposed,
>> >>>> >>>>>>>>>>> though,
>> >>>> >>>>>>>>>>> if I'm the only one seeing this issue, that means I've
>> >>>> >>>>>>>>>>> got
>> >>>> >>>>>>>>>>> something
>> >>>> >>>>>>>>>>> wrong somewhere.
>> >>>> >>>>>>>>>>>
>> >>>> >>>>>>>>>>
>> >>>> >>>>>>>>>>
>> >>>> >>>>>>>>>
>> >>>> >>>>>>>>>
>> >>>> >>>>>>>>>
>> >>>> >>>>>>>>> ------------------------------------------------------------------------------
>> >>>> >>>>>>>>> Come build with us! The BlackBerry® Developer
>> >>>> >>>>>>>>> Conference in
>> >>>> >>>>>>>>> SF, CA
>> >>>> >>>>>>>>> is the only developer event you need to attend this year.
>> >>>> >>>>>>>>> Jumpstart your
>> >>>> >>>>>>>>> developing skills, take BlackBerry mobile applications to
>> >>>> >>>>>>>>> market
>> >>>> >>>>>>>>> and stay
>> >>>> >>>>>>>>> ahead of the curve. Join us from November 9-12, 2009.
>> >>>> >>>>>>>>> Register now!
>> >>>> >>>>>>>>> http://p.sf.net/sfu/devconf
>> >>>> >>>>>>>>> --
>> >>>> >>>>>>>>> -----------------------------------------------
>> >>>> >>>>>>>>> jEdit Developers' List
>> >>>> >>>>>>>>> jEd...@li...
>> >>>> >>>>>>>>> https://lists.sourceforge.net/lists/listinfo/jedit-devel
>> >>>> >>>>>>>>>
>> >>>> >>>>>>>>
>> >>>> >>>>>>>
>> >>>> >>>>>>
>> >>>> >>>>>
>> >>>> >>>>
>> >>>> >>>
>> >>>> >>
>> >>>> >
>> >>>> >
>> >>>> >
>> >>>> > ------------------------------------------------------------------------------
>> >>>> > Come build with us! The BlackBerry® Developer Conference in SF,
>> >>>> > CA
>> >>>> > is the only developer event you need to attend this year. Jumpstart
>> >>>> > your
>> >>>> > developing skills, take BlackBerry mobile applications to market
>> >>>> > and
>> >>>> > stay
>> >>>> > ahead of the curve. Join us from November 9-12, 2009. Register
>> >>>> > now!
>> >>>> > http://p.sf.net/sfu/devconf
>> >>>> > --
>> >>>> > -----------------------------------------------
>> >>>> > jEdit Developers' List
>> >>>> > jEd...@li...
>> >>>> > https://lists.sourceforge.net/lists/listinfo/jedit-devel
>> >>>> >
>> >>>>
>> >>>>
>> >>>>
>> >>>> ------------------------------------------------------------------------------
>> >>>> Come build with us! The BlackBerry® Developer Conference in SF,
>> >>>> CA
>> >>>> is the only developer event you need to attend this year. Jumpstart
>> >>>> your
>> >>>> developing skills, take BlackBerry mobile applications to market and
>> >>>> stay
>> >>>> ahead of the curve. Join us from November 9-12, 2009. Register
>> >>>> now!
>> >>>> http://p.sf.net/sfu/devconf
>> >>>> --
>> >>>> -----------------------------------------------
>> >>>> jEdit Developers' List
>> >>>> jEd...@li...
>> >>>> https://lists.sourceforge.net/lists/listinfo/jedit-devel
>> >>>
>> >>>
>> >>
>> >
>>
>>
>> ------------------------------------------------------------------------------
>> Come build with us! The BlackBerry® Developer Conference in SF, CA
>> is the only developer event you need to attend this year. Jumpstart your
>> developing skills, take BlackBerry mobile applications to market and stay
>> ahead of the curve. Join us from November 9-12, 2009. Register
>> now!
>> http://p.sf.net/sfu/devconf
>> --
>> -----------------------------------------------
>> jEdit Developers' List
>> jEd...@li...
>> https://lists.sourceforge.net/lists/listinfo/jedit-devel
>>
>
>
> ------------------------------------------------------------------------------
> Come build with us! The BlackBerry® Developer Conference in SF, CA
> is the only developer event you need to attend this year. Jumpstart your
> developing skills, take BlackBerry mobile applications to market and stay
> ahead of the curve. Join us from November 9-12, 2009. Register now!
> http://p.sf.net/sfu/devconf
> --
> -----------------------------------------------
> jEdit Developers' List
> jEd...@li...
> https://lists.sourceforge.net/lists/listinfo/jedit-devel
>
>
|
|
From: Matthieu C. <cho...@gm...> - 2009-10-05 16:11:45
|
I don't understand that.
How can it be cached ? It is still recomputed at every call isn't it ?
2009/10/5 Shlomy Reinstein <sre...@gm...>
> Coming to think of it, I think that 'synchronized' is much better than
> the larger change. Why? Because typically, both the core and the
> plugins will call markTokens for the same line. So, the returned value
> of TokenMarker.markTokens will be cached in the line manager, and the
> 2nd of them that comes to ask for the tokens will get the cached data
> rather than re-computing it.
> If you don't make it synchronized, both will compute it at the same
> time, eventually one will override the other when the returned
> LineContext is saved in the line manager.
>
> Shlomy
>
> On Mon, Oct 5, 2009 at 5:47 PM, Matthieu Casanova
> <cho...@gm...> wrote:
> > Hi, that's what I thought about but you were quick.
> > Another change could be to have an instance of TokenMarkerData that would
> be
> > used if the caller thread is AWTThread, and if not create a new instance.
> > Since it will be AWTThread that is used most of the time it would save
> work
> > for the garbage collector, don't you think ?
> >
> > Matthieu
> >
> > 2009/10/5 Dale Anson <da...@gr...>
> >>
> >> Please see attached. I attached the whole file rather than a patch
> >> since the patch turned out to be quite large. There are a lot of
> >> small changes. Essentially, I turned the instance fields of
> >> TokenMarker into a data object, and am passing that data object
> >> around. Each call to markTokens gets its own data object, which keeps
> >> one call from interfering with the values being used from another
> >> call. I've been looking into how to write a unit test markTokens, and
> >> would like to get that done before committing a change like this.
> >>
> >> Dale
> >>
> >>
> >> On Sun, Oct 4, 2009 at 7:42 AM, Dale Anson <da...@gr...> wrote:
> >> > Making markTokens re-entrant would help performance in TaskList also.
> >> > TaskList uses a synchronized method to find tasks, the method is
> >> > synchronized to get around threading issues. I think if markTokens
> >> > was re-entrant, this TaskList method would not need to be
> >> > synchronized.
> >> >
> >> > It is more work to make markTokens re-entrant, but it looks to be
> >> > straight-forward to do. I'll give it a shot today and pass on the
> >> > results for review.
> >> >
> >> > Dale
> >> >
> >> >
> >> > On Sun, Oct 4, 2009 at 7:13 AM, Shlomy Reinstein <sre...@gm...>
> >> > wrote:
> >> >> I agree that making TokenMarker re-entrant rather than synchronized
> is
> >> >> better for performance. But like I wrote before, there are several
> >> >> fields that are modified by markTokens and its callees, and making it
> >> >> re-entrant is a larger change which might introduce another bug. If
> >> >> you'd like to do this yourself, feel free to...
> >> >>
> >> >> Unless you really can show a slowness in jEdit as a result of this,
> >> >> let's stay with the simple and safe fix. It fixes a real issue and I
> >> >> don't see it causing any.
> >> >>
> >> >> Shlomy
> >> >>
> >> >> On Sun, Oct 4, 2009 at 12:05 PM, Matthieu Casanova
> >> >> <cho...@gm...> wrote:
> >> >>> Hi in fact instead of making TokenMarker synchronized why not using
> >> >>> method
> >> >>> parameters instead of fields so the TokenMarker would be threadsafe
> >> >>> without
> >> >>> using synchronized code which is slow and since this code is an
> >> >>> important
> >> >>> point for performances I think it should be as fast as possible
> >> >>> Matthieu
> >> >>>
> >> >>> 2009/10/3 Shlomy Reinstein <sre...@gm...>
> >> >>>>
> >> >>>> It would (if possible), but that would require a larger change,
> which
> >> >>>> has a potential of introducing another bug. The one I did just adds
> >> >>>> the word "synchronized"... If there is a problem with that, I will
> >> >>>> consider another change. I am not completely sure it can be made
> >> >>>> reentrant, though.
> >> >>>>
> >> >>>> Shlomy
> >> >>>>
> >> >>>> On Sat, Oct 3, 2009 at 5:13 PM, Dale Anson <da...@gr...>
> >> >>>> wrote:
> >> >>>> > woops, just saw your other email about TaskList.
> >> >>>> >
> >> >>>> > Wouldn't it be better to make TokenMarker reentrant rather than
> >> >>>> > synchronized?
> >> >>>> >
> >> >>>> > Dale
> >> >>>> >
> >> >>>> >
> >> >>>> > On Sat, Oct 3, 2009 at 9:08 AM, Dale Anson <da...@gr...>
> >> >>>> > wrote:
> >> >>>> >> What was the issue with TaskList? And which TaskList -- the one
> >> >>>> >> in
> >> >>>> >> PluginManager or the one from Eric's daily builds?
> >> >>>> >>
> >> >>>> >> Dale
> >> >>>> >>
> >> >>>> >>
> >> >>>> >> On Sat, Oct 3, 2009 at 7:39 AM, Shlomy Reinstein
> >> >>>> >> <sre...@gm...>
> >> >>>> >> wrote:
> >> >>>> >>> I have committed this small change (make TokenMarker.markTokens
> >> >>>> >>> synchronized) and it seems to solve the issue with TaskList.
> >> >>>> >>> I also haven't noticed any performance impact of this. If
> anyone
> >> >>>> >>> discovers a performance problem caused by this, let me know and
> >> >>>> >>> I'll
> >> >>>> >>> fix it.
> >> >>>> >>>
> >> >>>> >>> Shlomy
> >> >>>> >>>
> >> >>>> >>> On Sat, Oct 3, 2009 at 11:40 AM, Shlomy Reinstein
> >> >>>> >>> <sre...@gm...>
> >> >>>> >>> wrote:
> >> >>>> >>>> Hi,
> >> >>>> >>>>
> >> >>>> >>>> It turns out that TokenMarker.markTokens is not thread-safe.
> It
> >> >>>> >>>> needs
> >> >>>> >>>> to be synchronized. It has a 'context' field, that is
> >> >>>> >>>> potentially
> >> >>>> >>>> updated each time "markTokens" is called. If called
> >> >>>> >>>> simultaneously by
> >> >>>> >>>> multiple threads, each may modify it, causing the other
> threads
> >> >>>> >>>> to
> >> >>>> >>>> use
> >> >>>> >>>> an incorrect context.
> >> >>>> >>>>
> >> >>>> >>>> I just verified that the problem happens very frequently with
> >> >>>> >>>> the
> >> >>>> >>>> TaskList plugin. TaskList has a dockable which shows the tasks
> >> >>>> >>>> in the
> >> >>>> >>>> current buffer, and also highlights tasks in the text area. To
> >> >>>> >>>> find
> >> >>>> >>>> the tasks in the buffer, it uses "markTokens" in the
> background.
> >> >>>> >>>> So,
> >> >>>> >>>> when this plugin is in use, I experience all kinds of painting
> >> >>>> >>>> issues
> >> >>>> >>>> and occasionally the NPE that Dale experienced.
> >> >>>> >>>> Disabling the TaskList plugin gets rid of all these problems.
> >> >>>> >>>>
> >> >>>> >>>> However, it is not a TaskList problem, "markTokens" simply
> needs
> >> >>>> >>>> to
> >> >>>> >>>> be
> >> >>>> >>>> synchronized. If this causes some performance problem in the
> >> >>>> >>>> core,
> >> >>>> >>>> then we should have two separate paths to markTokens, one for
> >> >>>> >>>> the
> >> >>>> >>>> core
> >> >>>> >>>> and one for plugins, and make the plugin's path ensure
> >> >>>> >>>> thread-safety,
> >> >>>> >>>> e.g. by deferring the task to the AWT thread.
> >> >>>> >>>>
> >> >>>> >>>> Shlomy
> >> >>>> >>>>
> >> >>>> >>>> On Sat, Oct 3, 2009 at 3:39 AM, Dale Anson <
> da...@gr...>
> >> >>>> >>>> wrote:
> >> >>>> >>>>> Actually, I verified that context.parent is occasionally
> null,
> >> >>>> >>>>> which
> >> >>>> >>>>> causes the NPE when its 'rules' field is referenced.
> Thinking
> >> >>>> >>>>> about
> >> >>>> >>>>> this some more, I bet is it's a threading issue. It could be
> >> >>>> >>>>> that
> >> >>>> >>>>> in
> >> >>>> >>>>> the first check (in the code you sent) it is not null, but is
> >> >>>> >>>>> reset
> >> >>>> >>>>> to
> >> >>>> >>>>> null before the method is actually invoked. That would mean
> my
> >> >>>> >>>>> patch
> >> >>>> >>>>> is not very good since it doesn't address thread-safety.
> >> >>>> >>>>>
> >> >>>> >>>>> I did double check, I've got source for jEdit at revision
> >> >>>> >>>>> 16233.
> >> >>>> >>>>> The
> >> >>>> >>>>> only change I have is the one I made to this class. I
> verified
> >> >>>> >>>>> that
> >> >>>> >>>>> I'm using the standard java mode from svn also, and it's
> while
> >> >>>> >>>>> editing
> >> >>>> >>>>> java files that I know I've seen the problem. I'm not sure
> if
> >> >>>> >>>>> I've
> >> >>>> >>>>> seen this issue in other modes.
> >> >>>> >>>>>
> >> >>>> >>>>> Dale
> >> >>>> >>>>>
> >> >>>> >>>>>
> >> >>>> >>>>> On Fri, Oct 2, 2009 at 2:46 AM, Shlomy Reinstein
> >> >>>> >>>>> <sre...@gm...> wrote:
> >> >>>> >>>>>> I took a more thorough look at the code. It is absolutely
> >> >>>> >>>>>> IMPOSSIBLE
> >> >>>> >>>>>> that context.parent is null in the method that generates the
> >> >>>> >>>>>> NPE
> >> >>>> >>>>>> for
> >> >>>> >>>>>> you, unless you've changed something in the jedit core.
> There
> >> >>>> >>>>>> is
> >> >>>> >>>>>> only
> >> >>>> >>>>>> one place where the method generating the NPE is called:
> >> >>>> >>>>>> //{{{ check for end of delegate
> >> >>>> >>>>>> if (context.parent != null
> >> >>>> >>>>>> && context.parent.inRule != null
> >> >>>> >>>>>> &&
> >> >>>> >>>>>> checkDelegateEnd(context.parent.inRule))
> >> >>>> >>>>>> {
> >> >>>> >>>>>> seenWhitespaceEnd = true;
> >> >>>> >>>>>> continue main_loop;
> >> >>>> >>>>>> } //}}}
> >> >>>> >>>>>>
> >> >>>> >>>>>> Note the check for "context.parent" not being null before
> >> >>>> >>>>>> calling
> >> >>>> >>>>>> this method.
> >> >>>> >>>>>> An NPE can still be caused by this line, but it won't be for
> >> >>>> >>>>>> "context.parent" being null, rather it would be for
> >> >>>> >>>>>> "context.parent.rules" being null.
> >> >>>> >>>>>>
> >> >>>> >>>>>> Shlomy
> >> >>>> >>>>>>
> >> >>>> >>>>>> On Fri, Oct 2, 2009 at 8:15 AM, Dale Anson
> >> >>>> >>>>>> <da...@gr...>
> >> >>>> >>>>>> wrote:
> >> >>>> >>>>>>> No, I can't reproduce it reliably. Somehow, I get into
> this
> >> >>>> >>>>>>> state
> >> >>>> >>>>>>> after a couple of hours of running jEdit. Usually, it
> >> >>>> >>>>>>> happens
> >> >>>> >>>>>>> when
> >> >>>> >>>>>>> I'm editing somewhere in the middle of a file, I'll hit
> Enter
> >> >>>> >>>>>>> at
> >> >>>> >>>>>>> the
> >> >>>> >>>>>>> end of a line to start the next line and then there is the
> >> >>>> >>>>>>> trouble. I
> >> >>>> >>>>>>> have verified that it is definitely the case that
> >> >>>> >>>>>>> context.parent
> >> >>>> >>>>>>> is
> >> >>>> >>>>>>> sometimes null. I see in LineContext itself that there are
> a
> >> >>>> >>>>>>> several
> >> >>>> >>>>>>> checks for a null parent, but not elsewhere. I think I've
> >> >>>> >>>>>>> only
> >> >>>> >>>>>>> seen
> >> >>>> >>>>>>> this with java files, but I'm not absolutely certain.
> >> >>>> >>>>>>>
> >> >>>> >>>>>>> Do you know of a better thing to do if context.parent is
> >> >>>> >>>>>>> null?
> >> >>>> >>>>>>>
> >> >>>> >>>>>>> Dale
> >> >>>> >>>>>>>
> >> >>>> >>>>>>>
> >> >>>> >>>>>>> On Thu, Oct 1, 2009 at 11:31 PM, Shlomy Reinstein
> >> >>>> >>>>>>> <sre...@gm...> wrote:
> >> >>>> >>>>>>>> Hi,
> >> >>>> >>>>>>>> Can you reproduce this every time with some example?
> >> >>>> >>>>>>>> If not , does this happen when you are editing (i.e.
> >> >>>> >>>>>>>> pressing
> >> >>>> >>>>>>>> enter
> >> >>>> >>>>>>>> on) the first or second line, or also when editing
> >> >>>> >>>>>>>> subsequent
> >> >>>> >>>>>>>> lines
> >> >>>> >>>>>>>> down the file?
> >> >>>> >>>>>>>> The 'parent' is usually the LineContext of the previous
> >> >>>> >>>>>>>> line.
> >> >>>> >>>>>>>> Shlomy
> >> >>>> >>>>>>>>
> >> >>>> >>>>>>>> On Fri, Oct 2, 2009 at 3:33 AM, Dale Anson
> >> >>>> >>>>>>>> <da...@gr...>
> >> >>>> >>>>>>>> wrote:
> >> >>>> >>>>>>>>> I entered a patch to fix this, see tracker 2871624. I
> did
> >> >>>> >>>>>>>>> some
> >> >>>> >>>>>>>>> more
> >> >>>> >>>>>>>>> investigation, and it turns out that context.parent can
> be
> >> >>>> >>>>>>>>> null.
> >> >>>> >>>>>>>>> The
> >> >>>> >>>>>>>>> patch checks for this case, and if the parent is null, it
> >> >>>> >>>>>>>>> just
> >> >>>> >>>>>>>>> continues to use the same context. So far, this seems to
> >> >>>> >>>>>>>>> be
> >> >>>> >>>>>>>>> working
> >> >>>> >>>>>>>>> fine.
> >> >>>> >>>>>>>>>
> >> >>>> >>>>>>>>> Dale
> >> >>>> >>>>>>>>>
> >> >>>> >>>>>>>>>
> >> >>>> >>>>>>>>> On Wed, Sep 30, 2009 at 1:07 PM, Alan Ezust
> >> >>>> >>>>>>>>> <ala...@gm...> wrote:
> >> >>>> >>>>>>>>>> I've seen an NPE there from time to time also, but I
> can't
> >> >>>> >>>>>>>>>> reproduce it.
> >> >>>> >>>>>>>>>>
> >> >>>> >>>>>>>>>> On Wed, Sep 30, 2009 at 11:50 AM, Dale Anson
> >> >>>> >>>>>>>>>> <da...@gr...> wrote:
> >> >>>> >>>>>>>>>>>
> >> >>>> >>>>>>>>>>> On Wed, Sep 30, 2009 at 9:58 AM, Matthieu Casanova
> >> >>>> >>>>>>>>>>> <cho...@gm...> wrote:
> >> >>>> >>>>>>>>>>> > But in that code, the fields are used as parameters,
> so
> >> >>>> >>>>>>>>>>> > filling the
> >> >>>> >>>>>>>>>>> > keyword
> >> >>>> >>>>>>>>>>> > field at line 294 is used to highlight tokens,
> removing
> >> >>>> >>>>>>>>>>> > that
> >> >>>> >>>>>>>>>>> > will lead
> >> >>>> >>>>>>>>>>> > to
> >> >>>> >>>>>>>>>>> > bugs.
> >> >>>> >>>>>>>>>>> > For the NPE at line 294, do you know if it is because
> >> >>>> >>>>>>>>>>> > of
> >> >>>> >>>>>>>>>>> > context being
> >> >>>> >>>>>>>>>>> > null
> >> >>>> >>>>>>>>>>> > or context.rules ?
> >> >>>> >>>>>>>>>>>
> >> >>>> >>>>>>>>>>> I don't know. I didn't look into it far enough. I
> >> >>>> >>>>>>>>>>> supposed,
> >> >>>> >>>>>>>>>>> though,
> >> >>>> >>>>>>>>>>> if I'm the only one seeing this issue, that means I've
> >> >>>> >>>>>>>>>>> got
> >> >>>> >>>>>>>>>>> something
> >> >>>> >>>>>>>>>>> wrong somewhere.
> >> >>>> >>>>>>>>>>>
> >> >>>> >>>>>>>>>>
> >> >>>> >>>>>>>>>>
> >> >>>> >>>>>>>>>
> >> >>>> >>>>>>>>>
> >> >>>> >>>>>>>>>
> >> >>>> >>>>>>>>>
> ------------------------------------------------------------------------------
> >> >>>> >>>>>>>>> Come build with us! The BlackBerry® Developer
> >> >>>> >>>>>>>>> Conference in
> >> >>>> >>>>>>>>> SF, CA
> >> >>>> >>>>>>>>> is the only developer event you need to attend this year.
> >> >>>> >>>>>>>>> Jumpstart your
> >> >>>> >>>>>>>>> developing skills, take BlackBerry mobile applications to
> >> >>>> >>>>>>>>> market
> >> >>>> >>>>>>>>> and stay
> >> >>>> >>>>>>>>> ahead of the curve. Join us from November 9-12, 2009.
> >> >>>> >>>>>>>>> Register now!
> >> >>>> >>>>>>>>> http://p.sf.net/sfu/devconf
> >> >>>> >>>>>>>>> --
> >> >>>> >>>>>>>>> -----------------------------------------------
> >> >>>> >>>>>>>>> jEdit Developers' List
> >> >>>> >>>>>>>>> jEd...@li...
> >> >>>> >>>>>>>>> https://lists.sourceforge.net/lists/listinfo/jedit-devel
> >> >>>> >>>>>>>>>
> >> >>>> >>>>>>>>
> >> >>>> >>>>>>>
> >> >>>> >>>>>>
> >> >>>> >>>>>
> >> >>>> >>>>
> >> >>>> >>>
> >> >>>> >>
> >> >>>> >
> >> >>>> >
> >> >>>> >
> >> >>>> >
> ------------------------------------------------------------------------------
> >> >>>> > Come build with us! The BlackBerry® Developer Conference in
> SF,
> >> >>>> > CA
> >> >>>> > is the only developer event you need to attend this year.
> Jumpstart
> >> >>>> > your
> >> >>>> > developing skills, take BlackBerry mobile applications to market
> >> >>>> > and
> >> >>>> > stay
> >> >>>> > ahead of the curve. Join us from November 9-12, 2009.
> Register
> >> >>>> > now!
> >> >>>> > http://p.sf.net/sfu/devconf
> >> >>>> > --
> >> >>>> > -----------------------------------------------
> >> >>>> > jEdit Developers' List
> >> >>>> > jEd...@li...
> >> >>>> > https://lists.sourceforge.net/lists/listinfo/jedit-devel
> >> >>>> >
> >> >>>>
> >> >>>>
> >> >>>>
> >> >>>>
> ------------------------------------------------------------------------------
> >> >>>> Come build with us! The BlackBerry® Developer Conference in SF,
> >> >>>> CA
> >> >>>> is the only developer event you need to attend this year. Jumpstart
> >> >>>> your
> >> >>>> developing skills, take BlackBerry mobile applications to market
> and
> >> >>>> stay
> >> >>>> ahead of the curve. Join us from November 9-12, 2009. Register
> >> >>>> now!
> >> >>>> http://p.sf.net/sfu/devconf
> >> >>>> --
> >> >>>> -----------------------------------------------
> >> >>>> jEdit Developers' List
> >> >>>> jEd...@li...
> >> >>>> https://lists.sourceforge.net/lists/listinfo/jedit-devel
> >> >>>
> >> >>>
> >> >>
> >> >
> >>
> >>
> >>
> ------------------------------------------------------------------------------
> >> Come build with us! The BlackBerry® Developer Conference in SF, CA
> >> is the only developer event you need to attend this year. Jumpstart your
> >> developing skills, take BlackBerry mobile applications to market and
> stay
> >> ahead of the curve. Join us from November 9-12, 2009. Register
> >> now!
> >> http://p.sf.net/sfu/devconf
> >> --
> >> -----------------------------------------------
> >> jEdit Developers' List
> >> jEd...@li...
> >> https://lists.sourceforge.net/lists/listinfo/jedit-devel
> >>
> >
> >
> >
> ------------------------------------------------------------------------------
> > Come build with us! The BlackBerry® Developer Conference in SF, CA
> > is the only developer event you need to attend this year. Jumpstart your
> > developing skills, take BlackBerry mobile applications to market and stay
> > ahead of the curve. Join us from November 9-12, 2009. Register
> now!
> > http://p.sf.net/sfu/devconf
> > --
> > -----------------------------------------------
> > jEdit Developers' List
> > jEd...@li...
> > https://lists.sourceforge.net/lists/listinfo/jedit-devel
> >
> >
>
|
Thanks for helping keep SourceForge clean.
X