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 >>> > >>> > >> >> > |