Menu
▾
▴
Re: [ jEdit-devel ] TokenMarker.java question
|
From: Matthieu C. <cho...@gm...> - 2009-10-05 16:11:45
|
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
> >
> >
>
|
