Re: [ jEdit-devel ] TokenMarker.java question

[Matthieu C. <cho...@gm...>]

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&reg; 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&#45;12, 2009.
> >>>> >>>>>>>>> Register now&#33;
> >>>> >>>>>>>>> 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&reg; 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&#45;12, 2009. Register
> >>>> > now&#33;
> >>>> > 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&reg; 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&#45;12, 2009. Register
> >>>> now&#33;
> >>>> 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&reg; 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&#45;12, 2009. Register now&#33;
> http://p.sf.net/sfu/devconf
> --
> -----------------------------------------------
> jEdit Developers' List
> jEd...@li...
> https://lists.sourceforge.net/lists/listinfo/jedit-devel
>
>