Re: [Dltk-dev] Buffer Syncronization After IFile.setContents()

[Shelby Sanders <ssanders@xxxxxxxxxxxx>]

Alex,

I also added a no-caching implementation (locally) and that's working  
for us.  I'll definitely integrate your changes and do some testing,  
hopefully the notification was just arriving late, like you say.

I was actually wondering if you had a chance to look at the IBuffer  
usage, based on my comments below.  I'm in the process of testing  
commenting out the following lines in  
AbstractSourceModule.buildStructure():
			// ensure buffer is opened
			if (hasBuffer()) {
				final IBuffer buffer = getBufferManager().getBuffer(this);
				if (buffer == null) {
					openBuffer(pm, moduleInfo);
				}
			}

So far, it doesn't cause any problems, but I still need to run our  
full test suite and take some before/after measurements.

Thank You,
Shelby Sanders


On Oct 15, 2008, at 03:51AM, Alex Panchenko wrote:

> Hi Shelby,
>
> Sorry for the delay. I have made the following changes today:
> - now there are 2 cache implementations - caching and no-caching.
> - preference and UI to select cache implementation, by default caching
> is turned off
> - resource change listener from the cache is registered in more  
> correct
> way, so this cache will be cleared a bit earlier (I suspect it was
> called too late in your configuration)
>
> I hope that even with enabled caching it would operate correctly in  
> your
> setup (would you please test it?), but anyway by default caching is
> disabled.
>
> Regards,
> Alex
>
>
> Shelby Sanders wrote:
> > Alex,
> >
> > Have you had a chance to look at this any further?
> >
> > I'm tempted to revert AbstractSourceModule.buildStructure() (at least
> > in our local source) to avoid creating a Buffer.  Do you think that
> > will cause any problems?
> >
> > Thank You,
> > Shelby Sanders
> >
> >
> > On Oct 2, 2008, at 07:18PM, Shelby Sanders wrote:
> >
> > > Alex,
> > >
> > > See responses inline.
> > >
> > > Thank You,
> > > Shelby Sanders
> > >
> > >
> > > On Oct 2, 2008, at 06:43PM, Alex Panchenko wrote:
> > >
> > > > Shelby,
> > > >
> > > > Buffer could be created by the ISourceModule.getBuffer() call even
> > > > without editor. Looking at the code - buffers are created only if
> > > > explicitly requested, general getSource*() calls do not create
> > > > buffers if buffer was not created before.
> > > >
> > > > hasBuffer() means that buffer could be created for this type of
> > > > elements, it does not check if buffer was already created or not.
> > >
> > > Maybe I'm misreading the code.  I see that
> > > AbstractSourceModule.buildStructure() does the following:
> > >       if (hasBuffer()) {
> > >               final IBuffer buffer =
> > > getBufferManager().getBuffer(this);
> > >               if (buffer == null) {
> > >                       openBuffer(pm, moduleInfo);
> > >               }
> > >       }
> > >
> > > And AbstractSourceModule also contains:
> > >       protected boolean hasBuffer() {
> > >               return true;
> > >       }
> > >
> > > My reading of this is that a buffer will always be created when
> > > buildStructure() is called, and that is almost guaranteed to happen
> > > for all ISourceModule instances.
> > >
> > > In fact, AbstractSourceModule.buildStructure() used to do the
> > > following specifically to avoid Buffer creation:
> > >       char[] contents;
> > >       IBuffer buffer = getBufferManager().getBuffer(this);
> > >       if (buffer != null) {
> > >               contents = buffer.getCharacters();
> > >       }
> > >       else {
> > >               //ssanders: PERFORMANCE - Avoid using a Buffer, if
> > > there isn't one
> > > already
> > >               contents = getBufferContent();
> > >               //buffer = openBuffer(pm, moduleInfo); // open buffer
> > >               // independently
> > >               // from the info, since we are building the info
> > >       }
> > >
> > > > I don't see the code to update the buffer if file content is
> > > > changed, probably it should not be updated so we don't lose  
> > > > possible
> > > > changes made in the buffer.
> > >
> > > I agree that we don't want to lose the changes in a dirty buffer.
> > >
> > > However, if IBuffer,hasUnsavedChanges() returns false, it should be
> > > safe to reload when the underlying IFile is changed.
> > >
> > > > TCL uses the same model classes (SourceModule and all others), but
> > > > buffers do not help with caching.
> > >
> > > If I'm reading the code correctly, then there is almost always an
> > > active buffer for all ISourceModule instances, and that's  
> > > basically a
> > > cache of the file content.
> > >
> > > > Would you please check when the removeFileEntry(IFile) and
> > > > get(IFile) methods are called (in the debugger and/or with the
> > > > printlns). Probably something is handling change event and
> > > > requesting content before the cache is cleared?
> > >
> > > As far as I can tell, SourceCodeCache is being updated correctly.
> > > However, the old file content is still in the Buffer, and the old  
> > > AST
> > > is still cached.
> > >
> > > At this point, it doesn't feel like SourceCodeCache is directly
> > > causing the problem, but if I comment out the caching, then things
> > > start working again.
> > >
> > > > I would like to get the exact picture before the final decision  
> > > > what
> > > > to do with the cache - make it more configurable or remove it
> > > > completely.
> > >
> > > I'm not sure that the cache needs to be removed all together.  We  
> > > just
> > > need to make sure that the invalidation strategy is covering all  
> > > cases.
> > >
> > > > Regards,
> > > > Alex
> > > >
> > > > ----- Original Message -----
> > > > From: "Shelby Sanders" <ssanders@xxxxxxxxxxxx>
> > > > To: "DLTK Developer Discussions" <dltk-dev@xxxxxxxxxxx>
> > > > Sent: Friday, October 3, 2008 5:04:50 AM GMT +06:00 Almaty,
> > > > Novosibirsk
> > > > Subject: Re: [Dltk-dev] Buffer Syncronization After
> > > > IFile.setContents()
> > > >
> > > > Alex,
> > > >
> > > > As far as I can tell, all of the events are received and handled by
> > > > SourceCodeCache$ChangeListener.visit(), and the SourceCodeCache  
> > > > is the
> > > > updated with the new content for the file.
> > > >
> > > > Therefore, it looks like the real problem is that the ISourceModule
> > > > has an active IBuffer which doesn't get updated with the new  
> > > > content.
> > > >
> > > > Thank You,
> > > > Shelby Sanders
> > > >
> > > >
> > > > On Oct 1, 2008, at 11:26PM, Alex Panchenko wrote:
> > > >
> > > > > Shelby,
> > > > >
> > > > > In the SourceCodeCache class there is inner ChangeListener class  
> > > > > to
> > > > > handle resource change events.
> > > > > The listener is added to the workspace in the constructor of the
> > > > > SourceCodeCache.
> > > > > Would you please test in the debugger if the events are  
> > > > > delivered to
> > > > > it
> > > > > in you setup?
> > > > > Probably some of the previously registered listeners throws an
> > > > > exception, so cache listener is not called?
> > > > >
> > > > > The cache was added to minimize file reads in networked  
> > > > > environments,
> > > > > you can disable it in your release, but I really would like to  
> > > > > know
> > > > > why
> > > > > it does not work as expected in your environment.
> > > > >
> > > > > Regards,
> > > > > Alex
> > > > >
> > > > > Shelby Sanders wrote:
> > > > > > Alex,
> > > > > >
> > > > > > See responses inline.
> > > > > >
> > > > > > Thank You,
> > > > > > Shelby Sanders
> > > > > >
> > > > > >
> > > > > > On Oct 1, 2008, at 01:28AM, Alex Panchenko wrote:
> > > > > >
> > > > > > > Hi Shelby,
> > > > > > >
> > > > > > > One of the recent changes (last Friday I think) was introduction
> > > > > > > of the
> > > > > > > IFile content caching.
> > > > > > > On change event the modified file is removed from cache.
> > > > > > > I have just committed the tests in the
> > > > > > > org.eclipse.dltk.ruby.core.tests.resources.SourceCacheTests  
> > > > > > > class.
> > > > > > > The test illustrates that after IFile.setContents() and
> > > > > > > IFile.delete()
> > > > > > > cache item is removed.
> > > > > >
> > > > > > I've updated to the latest sources from CVS HEAD.  I assume all  
> > > > > > of
> > > > > > the
> > > > > > tests are passing for you.
> > > > > >
> > > > > > SourceCacheTests.testResourceChange() is failing for me with the
> > > > > > following trace, which matches the behavior I'm seeing in our  
> > > > > > tests:
> > > > > > junit.framework.ComparisonFailure: expected:<class [Class]001
> > > > > > #
> > > > > > #
> > > > > > #
> > > > > > ...> but was:<class [Resource]001
> > > > > > #
> > > > > > #
> > > > > > #
> > > > > > ...>
> > > > > > at junit.framework.Assert.assertEquals(Assert.java:81)
> > > > > > at junit.framework.Assert.assertEquals(Assert.java:87)
> > > > > > at
> > > > > > org
> > > > > > .eclipse
> > > > > > .dltk
> > > > > > .ruby
> > > > > > .core
> > > > > > .tests
> > > > > > .resources
> > > > > > .SourceCacheTests.testResourceChange(SourceCacheTests.java:72)
> > > > > >
> > > > > > at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
> > > > > > at
> > > > > > sun
> > > > > > .reflect
> > > > > > .NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java: 
> > > > > > 39)
> > > > > >
> > > > > > at
> > > > > > sun
> > > > > > .reflect
> > > > > > .DelegatingMethodAccessorImpl
> > > > > > .invoke(DelegatingMethodAccessorImpl.java:25)
> > > > > >
> > > > > > at java.lang.reflect.Method.invoke(Method.java:597)
> > > > > > at junit.framework.TestCase.runTest(TestCase.java:164)
> > > > > > at junit.framework.TestCase.runBare(TestCase.java:130)
> > > > > > at junit.framework.TestResult$1.protect(TestResult.java:106)
> > > > > > at junit.framework.TestResult.runProtected(TestResult.java:124)
> > > > > > at junit.framework.TestResult.run(TestResult.java:109)
> > > > > > at junit.framework.TestCase.run(TestCase.java:120)
> > > > > > at junit.framework.TestSuite.runTest(TestSuite.java:230)
> > > > > > at
> > > > > > org.eclipse.dltk.core.tests.model.SuiteOfTestCases
> > > > > > $Suite.runTest(SuiteOfTestCases.java:151)
> > > > > >
> > > > > > at junit.framework.TestSuite.run(TestSuite.java:225)
> > > > > > at
> > > > > > org.eclipse.dltk.core.tests.model.SuiteOfTestCases
> > > > > > $Suite.superRun(SuiteOfTestCases.java:132)
> > > > > >
> > > > > > at
> > > > > > org.eclipse.dltk.core.tests.model.SuiteOfTestCases$Suite
> > > > > > $2.protect(SuiteOfTestCases.java:118)
> > > > > >
> > > > > > at junit.framework.TestResult.runProtected(TestResult.java:124)
> > > > > > at
> > > > > > org.eclipse.dltk.core.tests.model.SuiteOfTestCases
> > > > > > $Suite.run(SuiteOfTestCases.java:128)
> > > > > >
> > > > > > at junit.framework.TestSuite.runTest(TestSuite.java:230)
> > > > > > at junit.framework.TestSuite.run(TestSuite.java:225)
> > > > > > at
> > > > > > org
> > > > > > .eclipse
> > > > > > .jdt
> > > > > > .internal
> > > > > > .junit
> > > > > > .runner.junit3.JUnit3TestReference.run(JUnit3TestReference.java: 
> > > > > > 130)
> > > > > >
> > > > > > at
> > > > > > org
> > > > > > .eclipse
> > > > > > .jdt.internal.junit.runner.TestExecution.run(TestExecution.java: 
> > > > > > 38)
> > > > > >
> > > > > > at
> > > > > > org
> > > > > > .eclipse
> > > > > > .jdt
> > > > > > .internal
> > > > > > .junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java: 
> > > > > > 460)
> > > > > >
> > > > > > at
> > > > > > org
> > > > > > .eclipse
> > > > > > .jdt
> > > > > > .internal
> > > > > > .junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java: 
> > > > > > 673)
> > > > > >
> > > > > > at
> > > > > > org
> > > > > > .eclipse
> > > > > > .jdt
> > > > > > .internal 
> > > > > > .junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:
> > > > > > 386)
> > > > > >
> > > > > > at
> > > > > > org
> > > > > > .eclipse
> > > > > > .pde
> > > > > > .internal
> > > > > > .junit
> > > > > > .runtime 
> > > > > > .RemotePluginTestRunner.main(RemotePluginTestRunner.java:58)
> > > > > >
> > > > > > at
> > > > > > org.eclipse.pde.internal.junit.runtime.UITestApplication
> > > > > > $1.run(UITestApplication.java:122)
> > > > > >
> > > > > > at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35)
> > > > > > at
> > > > > > org
> > > > > > .eclipse
> > > > > > .swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:123)
> > > > > >
> > > > > > at
> > > > > > org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java: 
> > > > > > 3659)
> > > > > > at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:
> > > > > > 3296)
> > > > > > at
> > > > > > org.eclipse.ui.internal.Workbench.runEventLoop(Workbench.java: 
> > > > > > 2389)
> > > > > > at org.eclipse.ui.internal.Workbench.runUI(Workbench.java:2353)
> > > > > > at org.eclipse.ui.internal.Workbench.access$4(Workbench.java: 
> > > > > > 2219)
> > > > > > at org.eclipse.ui.internal.Workbench$4.run(Workbench.java:466)
> > > > > > at
> > > > > > org
> > > > > > .eclipse
> > > > > > .core.databinding.observable.Realm.runWithDefault(Realm.java:289)
> > > > > >
> > > > > > at
> > > > > > org
> > > > > > .eclipse 
> > > > > > .ui.internal.Workbench.createAndRunWorkbench(Workbench.java:
> > > > > > 461)
> > > > > >
> > > > > > at
> > > > > > org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java: 
> > > > > > 149)
> > > > > > at
> > > > > > org
> > > > > > .eclipse
> > > > > > .ui
> > > > > > .internal 
> > > > > > .ide.application.IDEApplication.start(IDEApplication.java:
> > > > > > 106)
> > > > > >
> > > > > > at
> > > > > > org
> > > > > > .eclipse
> > > > > > .pde
> > > > > > .internal
> > > > > > .junit.runtime.UITestApplication.start(UITestApplication.java:52)
> > > > > >
> > > > > > at
> > > > > > org
> > > > > > .eclipse
> > > > > > .equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:
> > > > > > 169)
> > > > > >
> > > > > > at
> > > > > > org
> > > > > > .eclipse
> > > > > > .core
> > > > > > .runtime
> > > > > > .internal
> > > > > > .adaptor 
> > > > > > .EclipseAppLauncher.runApplication(EclipseAppLauncher.java:
> > > > > > 106)
> > > > > >
> > > > > > at
> > > > > > org
> > > > > > .eclipse
> > > > > > .core
> > > > > > .runtime
> > > > > > .internal 
> > > > > > .adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:
> > > > > > 76)
> > > > > >
> > > > > > at
> > > > > > org
> > > > > > .eclipse
> > > > > > .core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:363)
> > > > > >
> > > > > > at
> > > > > > org
> > > > > > .eclipse
> > > > > > .core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:176)
> > > > > >
> > > > > > at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
> > > > > > at
> > > > > > sun
> > > > > > .reflect
> > > > > > .NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java: 
> > > > > > 39)
> > > > > >
> > > > > > at
> > > > > > sun
> > > > > > .reflect
> > > > > > .DelegatingMethodAccessorImpl
> > > > > > .invoke(DelegatingMethodAccessorImpl.java:25)
> > > > > >
> > > > > > at java.lang.reflect.Method.invoke(Method.java:597)
> > > > > > at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:
> > > > > > 508)
> > > > > > at org.eclipse.equinox.launcher.Main.basicRun(Main.java:447)
> > > > > > at org.eclipse.equinox.launcher.Main.run(Main.java:1173)
> > > > > > at org.eclipse.equinox.launcher.Main.main(Main.java:1148)
> > > > > >
> > > > > > > Another possible explanation is that you have active IBuffer for
> > > > > > > the
> > > > > > > SourceModule, so getSource() returns the buffer contents and
> > > > > > > modifications of the underlying resource are ignored.
> > > > > >
> > > > > > I don't think there should be an active buffer, because there  
> > > > > > isn't
> > > > > > an
> > > > > > editor open, since this happens in an automated test.
> > > > > >
> > > > > > Are there other cases where a buffer would be active?
> > > > > >
> > > > > > > You can temporary turn off the caching to check if it is related
> > > > > > > to your
> > > > > > > test failures (comment out Map.put or use null instead of  
> > > > > > > Map.get
> > > > > > > - in
> > > > > > > the future we are going to make pluggable cache providers for
> > > > > > > testing
> > > > > > > purposes, but at the moment the code modifications are  
> > > > > > > necessary).
> > > > > >
> > > > > > Yes, if I comment out the put() calls in SourceCodeCache, our  
> > > > > > tests
> > > > > > all pass again.
> > > > > >
> > > > > > Note, with the put() calls commented, I can get  
> > > > > > SourceCacheTests to
> > > > > > pass by commenting out both
> > > > > > "assertNotNull(cache.getContentsIfCached(file));" lines.
> > > > > >
> > > > > > > Could you please provide the code of the failing tests, so we  
> > > > > > > can
> > > > > > > check
> > > > > > > if it fails for us?
> > > > > >
> > > > > > I can't really provide the actual test code, because it depends  
> > > > > > on
> > > > > > quite a bit of our internal infrastructure.  However, the general
> > > > > > sequence of events is:
> > > > > > In TestCase.setUp():
> > > > > >     Use IFile.setContents() to update the contents of an already
> > > > > > existing *.rb file
> > > > > >     Call IProject.refreshLocal(IResource.DEPTH_INFINITE, new
> > > > > > NullProgressMonitor());
> > > > > >     Call
> > > > > > ResourcesPlugin
> > > > > > .getWorkspace 
> > > > > > ().build(IncrementalProjectBuilder.INCREMENTAL_BUILD,
> > > > > > new NullProgressMonitor());
> > > > > >     Wait for the build to finish, via calls to
> > > > > > AbstractModelTests.waitForAutoBuild() and
> > > > > > Job.getJobManager().join(ResourcesPlugin.FAMILY_MANUAL_BUILD,  
> > > > > > null);
> > > > > >     Wait for the indexer to finish, via a call to
> > > > > > AbstractModelTests.waitUntilIndexesReady()
> > > > > > Then in the test() method:
> > > > > >     Retrieve the IFile
> > > > > >     Retrieve the IModelElement for the IFile via
> > > > > > DLTKCore.create(file)
> > > > > >     Retrieve the AST for the file via
> > > > > > SourceParserUtil 
> > > > > > .getModuleDeclaration((ISourceModule)modelElement,
> > > > > > null);
> > > > > >     At this point we run some checks on the AST, which fail
> > > > > > because it is based on the old file contents.  I've confirmed  
> > > > > > this
> > > > > > by
> > > > > > calling ISourceModule.getSource().
> > > > > >
> > > > > > As you can see the logic is roughly the same as
> > > > > > SourceCacheTests.testResourceChange() with some additional  
> > > > > > logic to
> > > > > > retrieve and process the AST.
> > > > > >
> > > > > > Do you have any suggestions regarding why SourceCacheTests is
> > > > > > failing
> > > > > > for me and/or how to proceed with tracking this down?
> > > > > >
> > > > > > What part of the cache removal process is supposed to cause the
> > > > > > ISourceModule to reload from disk?
> > > > > >
> > > > > > > Regards,
> > > > > > > Alex
> > > > > > >
> > > > > > > Shelby Sanders wrote:
> > > > > > > > All,
> > > > > > > >
> > > > > > > > Something changed in the last week which is causing an  
> > > > > > > > interesting
> > > > > > > > issue in our automated unit tests.
> > > > > > > >
> > > > > > > > We use IFile.setContents() to update the content of a Ruby  
> > > > > > > > file,
> > > > > > > > and
> > > > > > > > then run various tests which access the ISourceModule for that
> > > > > > > > file.
> > > > > > > >
> > > > > > > > This was all happily working and everything stayed in sync,
> > > > > > > > until I
> > > > > > > > updated our code to use the latest sources from CVS HEAD as of
> > > > > > > > yesterday.
> > > > > > > >
> > > > > > > > Now, ISourceModule.getSource() still reports the old contents  
> > > > > > > > of
> > > > > > > > the
> > > > > > > > file, even after all the events have fired and the index is  
> > > > > > > > ready.
> > > > > > > > Also, calling ISourceModule.makeConsistent() doesn't help,  
> > > > > > > > because
> > > > > > > > ISourceModule.isConsistent() returns true causing it to return
> > > > > > > > without
> > > > > > > > doing anything.
> > > > > > > >
> > > > > > > > I've temporarily worked around the issue by calling
> > > > > > > > ISourceModule.close() then ISourceModule.open() after  
> > > > > > > > changing the
> > > > > > > > file contents.
> > > > > > > >
> > > > > > > > However, I'm guessing this is really just exposing a bigger
> > > > > > > > issue.  In
> > > > > > > > general, shouldn't ISourceModules detect when the underlying  
> > > > > > > > IFile
> > > > > > > > changes outside of the scope of DLTK, and update themselves
> > > > > > > > accordingly?
> > > > > > > >
> > > > > > > > Thank You,
> > > > > > > > Shelby Sanders
> > > > > > > > _______________________________________________
> > > > > > > > dltk-dev mailing list
> > > > > > > > dltk-dev@xxxxxxxxxxx
> > > > > > > > https://dev.eclipse.org/mailman/listinfo/dltk-dev
> > > > > > > _______________________________________________
> > > > > > > dltk-dev mailing list
> > > > > > > dltk-dev@xxxxxxxxxxx
> > > > > > > https://dev.eclipse.org/mailman/listinfo/dltk-dev
> > > > > >
> > > > > > _______________________________________________
> > > > > > dltk-dev mailing list
> > > > > > dltk-dev@xxxxxxxxxxx
> > > > > > https://dev.eclipse.org/mailman/listinfo/dltk-dev
> > > > > _______________________________________________
> > > > > dltk-dev mailing list
> > > > > dltk-dev@xxxxxxxxxxx
> > > > > https://dev.eclipse.org/mailman/listinfo/dltk-dev
> > > >
> > > > _______________________________________________
> > > > dltk-dev mailing list
> > > > dltk-dev@xxxxxxxxxxx
> > > > https://dev.eclipse.org/mailman/listinfo/dltk-dev
> > > > _______________________________________________
> > > > dltk-dev mailing list
> > > > dltk-dev@xxxxxxxxxxx
> > > > https://dev.eclipse.org/mailman/listinfo/dltk-dev
> > >
> > > _______________________________________________
> > > dltk-dev mailing list
> > > dltk-dev@xxxxxxxxxxx
> > > https://dev.eclipse.org/mailman/listinfo/dltk-dev
> >
> > _______________________________________________
> > dltk-dev mailing list
> > dltk-dev@xxxxxxxxxxx
> > https://dev.eclipse.org/mailman/listinfo/dltk-dev
> _______________________________________________
> dltk-dev mailing list
> dltk-dev@xxxxxxxxxxx
> https://dev.eclipse.org/mailman/listinfo/dltk-dev