From: Shlomy R. <sre...@gm...> - 2009-10-05 16:14:39
|
The line manager keeps track of the first line for which the context (i.e. tokens) has not been computed yet (firstInvalidLineContext). jEdit invalidates a line (i.e. if it's in the cache, sets it as the first invalid line context) when it's modified. If as a result of a line change, there are two synchronized calls to markTokens, the first one will compute it, and the 2nd one will use the value computed by the first one. See JEditBuffer.markTokens. Shlomy On Mon, Oct 5, 2009 at 6:11 PM, Matthieu Casanova <cho...@gm...> wrote: > 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 >> > >> > > > |
From: Shlomy R. <sre...@gm...> - 2009-10-05 16:15:54
|
One small correction: What I wrote below would happen if the 'synchronized' method is JEditBuffer.markTokens instead of TokenMarker.markTokens. Otherwise it might be as I wrote but might also not. Shlomy On Mon, Oct 5, 2009 at 6:14 PM, Shlomy Reinstein <sre...@gm...> wrote: > The line manager keeps track of the first line for which the context > (i.e. tokens) has not been computed yet (firstInvalidLineContext). > jEdit invalidates a line (i.e. if it's in the cache, sets it as the > first invalid line context) when it's modified. If as a result of a > line change, there are two synchronized calls to markTokens, the first > one will compute it, and the 2nd one will use the value computed by > the first one. See JEditBuffer.markTokens. > > Shlomy > > On Mon, Oct 5, 2009 at 6:11 PM, Matthieu Casanova > <cho...@gm...> wrote: >> 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 >>> > >>> > >> >> > |
From: Dale A. <da...@gr...> - 2009-10-05 17:57:14
|
I did do some profiling on all 3 methods. I ran a set of 3890 files through markTokens with these results: Original TokenMarker.java, revision 16233: 5376 ms Synchronized TokenMarker.java: 5968 ms Thread-safe TokenMarker.java: 5747 All three methods are very fast. On Mon, Oct 5, 2009 at 10:15 AM, Shlomy Reinstein <sre...@gm...> wrote: > One small correction: What I wrote below would happen if the > 'synchronized' method is JEditBuffer.markTokens instead of > TokenMarker.markTokens. Otherwise it might be as I wrote but might > also not. > > Shlomy > > On Mon, Oct 5, 2009 at 6:14 PM, Shlomy Reinstein <sre...@gm...> wrote: >> The line manager keeps track of the first line for which the context >> (i.e. tokens) has not been computed yet (firstInvalidLineContext). >> jEdit invalidates a line (i.e. if it's in the cache, sets it as the >> first invalid line context) when it's modified. If as a result of a >> line change, there are two synchronized calls to markTokens, the first >> one will compute it, and the 2nd one will use the value computed by >> the first one. See JEditBuffer.markTokens. >> >> Shlomy >> >> On Mon, Oct 5, 2009 at 6:11 PM, Matthieu Casanova >> <cho...@gm...> wrote: >>> 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 >>>> > >>>> > >>> >>> >> > |
From: Dale A. <da...@gr...> - 2009-10-12 14:02:57
|
As far as I am concerned, this matter is closed. Shlomy's change of adding 'synchronized' to the markTokens method has fixed the problem. I have not seen this problem again since that change. I've ran additional performance tests, and which version is faster than the other changes with each run it seems. All are very fast, so I don't think it is worth the trouble at this point to introduce the more significant change of making TokenManager reentrant. Dale On Mon, Oct 5, 2009 at 11:57 AM, Dale Anson <da...@gr...> wrote: > I did do some profiling on all 3 methods. I ran a set of 3890 files > through markTokens with these results: > > Original TokenMarker.java, revision 16233: 5376 ms > Synchronized TokenMarker.java: 5968 ms > Thread-safe TokenMarker.java: 5747 > > All three methods are very fast. > > > On Mon, Oct 5, 2009 at 10:15 AM, Shlomy Reinstein <sre...@gm...> wrote: >> One small correction: What I wrote below would happen if the >> 'synchronized' method is JEditBuffer.markTokens instead of >> TokenMarker.markTokens. Otherwise it might be as I wrote but might >> also not. >> >> Shlomy >> >> On Mon, Oct 5, 2009 at 6:14 PM, Shlomy Reinstein <sre...@gm...> wrote: >>> The line manager keeps track of the first line for which the context >>> (i.e. tokens) has not been computed yet (firstInvalidLineContext). >>> jEdit invalidates a line (i.e. if it's in the cache, sets it as the >>> first invalid line context) when it's modified. If as a result of a >>> line change, there are two synchronized calls to markTokens, the first >>> one will compute it, and the 2nd one will use the value computed by >>> the first one. See JEditBuffer.markTokens. >>> >>> Shlomy >>> >>> On Mon, Oct 5, 2009 at 6:11 PM, Matthieu Casanova >>> <cho...@gm...> wrote: >>>> 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 >>>>> > >>>>> > >>>> >>>> >>> >> > |
From: Shlomy R. <sre...@gm...> - 2009-10-12 14:17:09
|
I strongly agree. As I wrote in a previous email, even moving the "synchronized" word one level up, to JEditBuffer.markTokens (which is the only caller of TokenMarker.markTokens), may result in additional performance if several callers want to mark tokens for the same lines. Shlomy On Mon, Oct 12, 2009 at 4:02 PM, Dale Anson <da...@gr...> wrote: > As far as I am concerned, this matter is closed. Shlomy's change of > adding 'synchronized' to the markTokens method has fixed the problem. > I have not seen this problem again since that change. I've ran > additional performance tests, and which version is faster than the > other changes with each run it seems. All are very fast, so I don't > think it is worth the trouble at this point to introduce the more > significant change of making TokenManager reentrant. > > Dale > > > On Mon, Oct 5, 2009 at 11:57 AM, Dale Anson <da...@gr...> wrote: >> I did do some profiling on all 3 methods. I ran a set of 3890 files >> through markTokens with these results: >> >> Original TokenMarker.java, revision 16233: 5376 ms >> Synchronized TokenMarker.java: 5968 ms >> Thread-safe TokenMarker.java: 5747 >> >> All three methods are very fast. >> >> >> On Mon, Oct 5, 2009 at 10:15 AM, Shlomy Reinstein <sre...@gm...> wrote: >>> One small correction: What I wrote below would happen if the >>> 'synchronized' method is JEditBuffer.markTokens instead of >>> TokenMarker.markTokens. Otherwise it might be as I wrote but might >>> also not. >>> >>> Shlomy >>> >>> On Mon, Oct 5, 2009 at 6:14 PM, Shlomy Reinstein <sre...@gm...> wrote: >>>> The line manager keeps track of the first line for which the context >>>> (i.e. tokens) has not been computed yet (firstInvalidLineContext). >>>> jEdit invalidates a line (i.e. if it's in the cache, sets it as the >>>> first invalid line context) when it's modified. If as a result of a >>>> line change, there are two synchronized calls to markTokens, the first >>>> one will compute it, and the 2nd one will use the value computed by >>>> the first one. See JEditBuffer.markTokens. >>>> >>>> Shlomy >>>> >>>> On Mon, Oct 5, 2009 at 6:11 PM, Matthieu Casanova >>>> <cho...@gm...> wrote: >>>>> 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 >>>>>> > >>>>>> > >>>>> >>>>> >>>> >>> >> > > ------------------------------------------------------------------------------ > Come build with us! The BlackBerry(R) 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/devconference > -- > ----------------------------------------------- > jEdit Developers' List > jEd...@li... > https://lists.sourceforge.net/lists/listinfo/jedit-devel > |