Menu
▾
▴
Re: [ jEdit-devel ] TokenMarker.java question
|
From: Dale A. <da...@gr...> - 2009-10-05 15:34:35
|
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
>>>
>>>
>>
>
|
