From: Shlomy R. <sre...@gm...> - 2009-10-04 13:13:21
|
I agree that making TokenMarker re-entrant rather than synchronized is better for performance. But like I wrote before, there are several fields that are modified by markTokens and its callees, and making it re-entrant is a larger change which might introduce another bug. If you'd like to do this yourself, feel free to... Unless you really can show a slowness in jEdit as a result of this, let's stay with the simple and safe fix. It fixes a real issue and I don't see it causing any. Shlomy On Sun, Oct 4, 2009 at 12:05 PM, Matthieu Casanova <cho...@gm...> wrote: > Hi in fact instead of making TokenMarker synchronized why not using method > parameters instead of fields so the TokenMarker would be threadsafe without > using synchronized code which is slow and since this code is an important > point for performances I think it should be as fast as possible > Matthieu > > 2009/10/3 Shlomy Reinstein <sre...@gm...> >> >> It would (if possible), but that would require a larger change, which >> has a potential of introducing another bug. The one I did just adds >> the word "synchronized"... If there is a problem with that, I will >> consider another change. I am not completely sure it can be made >> reentrant, though. >> >> Shlomy >> >> On Sat, Oct 3, 2009 at 5:13 PM, Dale Anson <da...@gr...> wrote: >> > woops, just saw your other email about TaskList. >> > >> > Wouldn't it be better to make TokenMarker reentrant rather than >> > synchronized? >> > >> > Dale >> > >> > >> > On Sat, Oct 3, 2009 at 9:08 AM, Dale Anson <da...@gr...> wrote: >> >> What was the issue with TaskList? And which TaskList -- the one in >> >> PluginManager or the one from Eric's daily builds? >> >> >> >> Dale >> >> >> >> >> >> On Sat, Oct 3, 2009 at 7:39 AM, Shlomy Reinstein <sre...@gm...> >> >> wrote: >> >>> I have committed this small change (make TokenMarker.markTokens >> >>> synchronized) and it seems to solve the issue with TaskList. >> >>> I also haven't noticed any performance impact of this. If anyone >> >>> discovers a performance problem caused by this, let me know and I'll >> >>> fix it. >> >>> >> >>> Shlomy >> >>> >> >>> On Sat, Oct 3, 2009 at 11:40 AM, Shlomy Reinstein <sre...@gm...> >> >>> wrote: >> >>>> Hi, >> >>>> >> >>>> It turns out that TokenMarker.markTokens is not thread-safe. It needs >> >>>> to be synchronized. It has a 'context' field, that is potentially >> >>>> updated each time "markTokens" is called. If called simultaneously by >> >>>> multiple threads, each may modify it, causing the other threads to >> >>>> use >> >>>> an incorrect context. >> >>>> >> >>>> I just verified that the problem happens very frequently with the >> >>>> TaskList plugin. TaskList has a dockable which shows the tasks in the >> >>>> current buffer, and also highlights tasks in the text area. To find >> >>>> the tasks in the buffer, it uses "markTokens" in the background. So, >> >>>> when this plugin is in use, I experience all kinds of painting issues >> >>>> and occasionally the NPE that Dale experienced. >> >>>> Disabling the TaskList plugin gets rid of all these problems. >> >>>> >> >>>> However, it is not a TaskList problem, "markTokens" simply needs to >> >>>> be >> >>>> synchronized. If this causes some performance problem in the core, >> >>>> then we should have two separate paths to markTokens, one for the >> >>>> core >> >>>> and one for plugins, and make the plugin's path ensure thread-safety, >> >>>> e.g. by deferring the task to the AWT thread. >> >>>> >> >>>> Shlomy >> >>>> >> >>>> On Sat, Oct 3, 2009 at 3:39 AM, Dale Anson <da...@gr...> >> >>>> wrote: >> >>>>> Actually, I verified that context.parent is occasionally null, which >> >>>>> causes the NPE when its 'rules' field is referenced. Thinking about >> >>>>> this some more, I bet is it's a threading issue. It could be that >> >>>>> in >> >>>>> the first check (in the code you sent) it is not null, but is reset >> >>>>> to >> >>>>> null before the method is actually invoked. That would mean my >> >>>>> patch >> >>>>> is not very good since it doesn't address thread-safety. >> >>>>> >> >>>>> I did double check, I've got source for jEdit at revision 16233. >> >>>>> The >> >>>>> only change I have is the one I made to this class. I verified that >> >>>>> I'm using the standard java mode from svn also, and it's while >> >>>>> editing >> >>>>> java files that I know I've seen the problem. I'm not sure if I've >> >>>>> seen this issue in other modes. >> >>>>> >> >>>>> Dale >> >>>>> >> >>>>> >> >>>>> On Fri, Oct 2, 2009 at 2:46 AM, Shlomy Reinstein >> >>>>> <sre...@gm...> wrote: >> >>>>>> I took a more thorough look at the code. It is absolutely >> >>>>>> IMPOSSIBLE >> >>>>>> that context.parent is null in the method that generates the NPE >> >>>>>> for >> >>>>>> you, unless you've changed something in the jedit core. There is >> >>>>>> only >> >>>>>> one place where the method generating the NPE is called: >> >>>>>> //{{{ check for end of delegate >> >>>>>> if (context.parent != null >> >>>>>> && context.parent.inRule != null >> >>>>>> && >> >>>>>> checkDelegateEnd(context.parent.inRule)) >> >>>>>> { >> >>>>>> seenWhitespaceEnd = true; >> >>>>>> continue main_loop; >> >>>>>> } //}}} >> >>>>>> >> >>>>>> Note the check for "context.parent" not being null before calling >> >>>>>> this method. >> >>>>>> An NPE can still be caused by this line, but it won't be for >> >>>>>> "context.parent" being null, rather it would be for >> >>>>>> "context.parent.rules" being null. >> >>>>>> >> >>>>>> Shlomy >> >>>>>> >> >>>>>> On Fri, Oct 2, 2009 at 8:15 AM, Dale Anson <da...@gr...> >> >>>>>> wrote: >> >>>>>>> No, I can't reproduce it reliably. Somehow, I get into this state >> >>>>>>> after a couple of hours of running jEdit. Usually, it happens >> >>>>>>> when >> >>>>>>> I'm editing somewhere in the middle of a file, I'll hit Enter at >> >>>>>>> the >> >>>>>>> end of a line to start the next line and then there is the >> >>>>>>> trouble. I >> >>>>>>> have verified that it is definitely the case that context.parent >> >>>>>>> is >> >>>>>>> sometimes null. I see in LineContext itself that there are a >> >>>>>>> several >> >>>>>>> checks for a null parent, but not elsewhere. I think I've only >> >>>>>>> seen >> >>>>>>> this with java files, but I'm not absolutely certain. >> >>>>>>> >> >>>>>>> Do you know of a better thing to do if context.parent is null? >> >>>>>>> >> >>>>>>> Dale >> >>>>>>> >> >>>>>>> >> >>>>>>> On Thu, Oct 1, 2009 at 11:31 PM, Shlomy Reinstein >> >>>>>>> <sre...@gm...> wrote: >> >>>>>>>> Hi, >> >>>>>>>> Can you reproduce this every time with some example? >> >>>>>>>> If not , does this happen when you are editing (i.e. pressing >> >>>>>>>> enter >> >>>>>>>> on) the first or second line, or also when editing subsequent >> >>>>>>>> lines >> >>>>>>>> down the file? >> >>>>>>>> The 'parent' is usually the LineContext of the previous line. >> >>>>>>>> Shlomy >> >>>>>>>> >> >>>>>>>> On Fri, Oct 2, 2009 at 3:33 AM, Dale Anson <da...@gr...> >> >>>>>>>> wrote: >> >>>>>>>>> I entered a patch to fix this, see tracker 2871624. I did some >> >>>>>>>>> more >> >>>>>>>>> investigation, and it turns out that context.parent can be null. >> >>>>>>>>> The >> >>>>>>>>> patch checks for this case, and if the parent is null, it just >> >>>>>>>>> continues to use the same context. So far, this seems to be >> >>>>>>>>> working >> >>>>>>>>> fine. >> >>>>>>>>> >> >>>>>>>>> Dale >> >>>>>>>>> >> >>>>>>>>> >> >>>>>>>>> On Wed, Sep 30, 2009 at 1:07 PM, Alan Ezust >> >>>>>>>>> <ala...@gm...> wrote: >> >>>>>>>>>> I've seen an NPE there from time to time also, but I can't >> >>>>>>>>>> reproduce it. >> >>>>>>>>>> >> >>>>>>>>>> On Wed, Sep 30, 2009 at 11:50 AM, Dale Anson >> >>>>>>>>>> <da...@gr...> wrote: >> >>>>>>>>>>> >> >>>>>>>>>>> On Wed, Sep 30, 2009 at 9:58 AM, Matthieu Casanova >> >>>>>>>>>>> <cho...@gm...> wrote: >> >>>>>>>>>>> > But in that code, the fields are used as parameters, so >> >>>>>>>>>>> > filling the >> >>>>>>>>>>> > keyword >> >>>>>>>>>>> > field at line 294 is used to highlight tokens, removing that >> >>>>>>>>>>> > will lead >> >>>>>>>>>>> > to >> >>>>>>>>>>> > bugs. >> >>>>>>>>>>> > For the NPE at line 294, do you know if it is because of >> >>>>>>>>>>> > context being >> >>>>>>>>>>> > null >> >>>>>>>>>>> > or context.rules ? >> >>>>>>>>>>> >> >>>>>>>>>>> I don't know. I didn't look into it far enough. I supposed, >> >>>>>>>>>>> though, >> >>>>>>>>>>> if I'm the only one seeing this issue, that means I've got >> >>>>>>>>>>> something >> >>>>>>>>>>> wrong somewhere. >> >>>>>>>>>>> >> >>>>>>>>>> >> >>>>>>>>>> >> >>>>>>>>> >> >>>>>>>>> >> >>>>>>>>> ------------------------------------------------------------------------------ >> >>>>>>>>> Come build with us! The BlackBerry® Developer Conference in >> >>>>>>>>> SF, CA >> >>>>>>>>> is the only developer event you need to attend this year. >> >>>>>>>>> Jumpstart your >> >>>>>>>>> developing skills, take BlackBerry mobile applications to market >> >>>>>>>>> and stay >> >>>>>>>>> ahead of the curve. Join us from November 9-12, 2009. >> >>>>>>>>> Register now! >> >>>>>>>>> http://p.sf.net/sfu/devconf >> >>>>>>>>> -- >> >>>>>>>>> ----------------------------------------------- >> >>>>>>>>> jEdit Developers' List >> >>>>>>>>> jEd...@li... >> >>>>>>>>> https://lists.sourceforge.net/lists/listinfo/jedit-devel >> >>>>>>>>> >> >>>>>>>> >> >>>>>>> >> >>>>>> >> >>>>> >> >>>> >> >>> >> >> >> > >> > >> > ------------------------------------------------------------------------------ >> > Come build with us! The BlackBerry® Developer Conference in SF, CA >> > is the only developer event you need to attend this year. Jumpstart your >> > developing skills, take BlackBerry mobile applications to market and >> > stay >> > ahead of the curve. Join us from November 9-12, 2009. Register >> > now! >> > http://p.sf.net/sfu/devconf >> > -- >> > ----------------------------------------------- >> > jEdit Developers' List >> > jEd...@li... >> > https://lists.sourceforge.net/lists/listinfo/jedit-devel >> > >> >> >> ------------------------------------------------------------------------------ >> Come build with us! The BlackBerry® Developer Conference in SF, CA >> is the only developer event you need to attend this year. Jumpstart your >> developing skills, take BlackBerry mobile applications to market and stay >> ahead of the curve. Join us from November 9-12, 2009. Register >> now! >> http://p.sf.net/sfu/devconf >> -- >> ----------------------------------------------- >> jEdit Developers' List >> jEd...@li... >> https://lists.sourceforge.net/lists/listinfo/jedit-devel > > |