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