Menu
▾
▴
Re: [ jEdit-devel ] TokenMarker.java question
|
From: Shlomy R. <sre...@gm...> - 2009-10-05 15:42:46
|
That's nice, but did you check the performance? Before replacing a
very simple fix like 'synchronized' with such a large fix (even if
trivial), do you have some performance benchmarking results that show
that your fix improves performance?
The performance cost of creating a new data object and copying the
values into its members might be even larger than synchronizing the
method...
Shlomy
On Mon, Oct 5, 2009 at 5:34 PM, Dale Anson <da...@gr...> wrote:
> 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
>
>
|