Closed Bug 828223 Opened 10 years ago Closed 10 years ago

nsSearchService write cache using OS.File

Categories

(Firefox :: Search, defect)

Product:
Firefox
Component:
Search
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 22

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

Attachments

(1 file, 3 obsolete files)

Write nsSearchService cache with OS.File, v2.2
8.10 KB, patch
Yoric
: review+
Details | Diff | Splinter Review 
Extracted from bug 706523.
Migrate writing the cache to OS.File.
Assignee: nobody → dteller

        Attachment #699732 -
        Flags: review?(gavin.sharp)

    
    

    
  
Comment on attachment 699732 [details] [diff] [review]
Write nsSearchService cache with OS.File

Do we have test coverage here?

        Attachment #699732 -
        Flags: review?(gavin.sharp) → review+

    
    

    
  
I don't think we do. I'll try and find out how to add some.

    
    

    
  
So, for the sake of testing, I could send a notification "cache-built" or something such. Gavin, do you think it's worth the bother?
Flags: needinfo?(gavin.sharp)

    
    

    
  
If that's all that's needed to get test coverage for cache population that we otherwise don't have, that certainly sounds worth it!
Flags: needinfo?(gavin.sharp)

    
    

    
  


  
Same one, with tests.

        Attachment #699732 -
        Attachment is obsolete: true

        Attachment #701066 -
        Flags: review?(gavin.sharp)

        Attachment #701066 -
        Flags: review?(gavin.sharp) → review+

    
    

    
  
Hrm, was this forgotten, or is there something else preventing it from landing?

    
    

    
  


  
Indeed, it was forgotten (and slightly bitrotten).

I will wait for Telemetry to tell us whether encoding on the main thread is a good idea, though. I am starting to believe that we should wait for something like bug 832664 before proceeding with such transformations.

        Attachment #701066 -
        Attachment is obsolete: true

        Attachment #716529 -
        Flags: review+

    
    

    
  
(In reply to David Rajchenbach Teller [:Yoric] from comment #8)
> I will wait for Telemetry to tell us whether encoding on the main thread is
> a good idea, though. I am starting to believe that we should wait for
> something like bug 832664 before proceeding with such transformations.

This isn't adding any additional main thread work, right? As far as I can tell this is a net reduction in the amount of main-thread work going on, so I don't see why we would block on additional improvements.

    
    

    
  
Ah, you are right. I was assuming that the pre-existing code managed to make more things asynchronous. So the patch is a net improvement – one which we may want to improve further.

Try: https://tbpl.mozilla.org/?tree=Try&rev=a1ecaeb21261

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/910837eec45a
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22

    
    

    
  
Hmm, this test fails intermittently when we run it on our own builds in Ubuntu:

https://jenkins.qa.ubuntu.com/job/raring-ppa-adt-ubuntu_mozilla_daily_ppa-firefox-trunk/92/ARCH=amd64,label=adt/testReport/junit/toolkit.components.search.tests/xpcshell/test_nocache_js/

It looks like adding the search engine completes before the initial cache file is written

    
    

    
  
I'll take a look asap.
Flags: needinfo?(dteller)

    
    

    
  
Chris, I can't read that log. Do you still have it accessible somewhere?
Flags: needinfo?(dteller) → needinfo?(chrisccoulson)

    
    

    
  
Ah, sorry, I didn't realize that Jenkins doesn't keep very much history, otherwise I would have just pasted the failure inline (and we filter this test out now so that the failure doesn't show up in the UI).

I've just had a look through some recent logs though and found this, which I think is the same failure:

TEST-INFO | /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_nocache.js | running test ...
TEST-UNEXPECTED-FAIL | /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_nocache.js | test failed (with xpcshell return code: 0), see following log:
>>>>>>>

TEST-INFO | (xpcshell/head.js) | test 1 pending

TEST-PASS | /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/head.js | [do_load_manifest : 834] true == true

TEST-INFO | (xpcshell/head.js) | test 2 pending
*** Search: metadata init: starting

TEST-INFO | (xpcshell/head.js) | test 2 finished

TEST-INFO | (xpcshell/head.js) | running event loop
*** Search: SRCH_SVC_EMS_asyncMigrate start
*** Search: SRCH_SVC_EMS_migrate search.sqlite does not exist
*** Search: metadata init: No store to migrate to disk
*** Search: _syncLoadEngines: start
*** Search: _loadEngines: Absent or outdated cache. Loading engines from disk.
*** Search: _buildCache: Writing to cache file.

TEST-PASS | /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_nocache.js | [ss_initialized : 45] true == true

TEST-INFO | /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_nocache.js | Setting up observer
*** Search: addEngine: Adding "http://localhost:4444/data/engine.xml".
*** Search: _initFromURI: Downloading engine from: "http://localhost:4444/data/engine.xml".
*** Search: _syncInit: Completed _syncInit
*** Search: loadListener: Starting request: http://localhost:4444/data/engine.xml
*** Search: loadListener: Stopping request: http://localhost:4444/data/engine.xml
*** Search: _init: Initing MozSearch plugin from http://localhost:4444/data/engine.xml
*** Search: _parseImage: Image textContent: "data:image/png;base64,AAABAAEAEBAAAAEAGABoAwAAFgAAACgAAAAQAAAAIAAAAAEAGAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAADs9Pt8xetPtu9FsfFNtu%2BTzvb2%2B%2Fne4dFJeBw0egA%2FfAJAfAA8ewBBegAAAAD%2B%2FPtft98Mp%2BwWsfAVsvEbs%2FQeqvF8xO7%2F%2F%2F63yqkxdgM7gwE%2FggM%2BfQA%2BegBDeQDe7PIbotgQufcMufEPtfIPsvAbs%2FQvq%2Bfz%2Bf%2F%2B%2B%2FZKhR05hgBBhQI8hgBAgAI9ewD0%2B%2Fg3pswAtO8Cxf4Kw%2FsJvvYAqupKsNv%2B%2Fv7%2F%2FP5VkSU0iQA7jQA9hgBDgQU%2BfQH%2F%2Ff%2FQ6fM4sM4KsN8AteMCruIqqdbZ7PH8%2Fv%2Fg6Nc%2Fhg05kAA8jAM9iQI%2BhQA%2BgQDQu6b97uv%2F%2F%2F7V8Pqw3eiWz97q8%2Ff%2F%2F%2F%2F7%2FPptpkkqjQE4kwA7kAA5iwI8iAA8hQCOSSKdXjiyflbAkG7u2s%2F%2B%2F%2F39%2F%2F7r8utrqEYtjQE8lgA7kwA7kwA9jwA9igA9hACiWSekVRyeSgiYSBHx6N%2F%2B%2Fv7k7OFRmiYtlAA5lwI7lwI4lAA7kgI9jwE9iwI4iQCoVhWcTxCmb0K%2BooT8%2Fv%2F7%2F%2F%2FJ2r8fdwI1mwA3mQA3mgA8lAE8lAE4jwA9iwE%2BhwGfXifWvqz%2B%2Ff%2F58u%2Fev6Dt4tr%2B%2F%2F2ZuIUsggA7mgM6mAM3lgA5lgA6kQE%2FkwBChwHt4dv%2F%2F%2F728ei1bCi7VAC5XQ7kz7n%2F%2F%2F6bsZkgcB03lQA9lgM7kwA2iQktZToPK4r9%2F%2F%2F9%2F%2F%2FSqYK5UwDKZAS9WALIkFn%2B%2F%2F3%2F%2BP8oKccGGcIRJrERILYFEMwAAuEAAdX%2F%2Ff7%2F%2FP%2B%2BfDvGXQLIZgLEWgLOjlf7%2F%2F%2F%2F%2F%2F9QU90EAPQAAf8DAP0AAfMAAOUDAtr%2F%2F%2F%2F7%2B%2Fu2bCTIYwDPZgDBWQDSr4P%2F%2Fv%2F%2F%2FP5GRuABAPkAA%2FwBAfkDAPAAAesAAN%2F%2F%2B%2Fz%2F%2F%2F64g1C5VwDMYwK8Yg7y5tz8%2Fv%2FV1PYKDOcAAP0DAf4AAf0AAfYEAOwAAuAAAAD%2F%2FPvi28ymXyChTATRrIb8%2F%2F3v8fk6P8MAAdUCAvoAAP0CAP0AAfYAAO4AAACAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACAAQAA"
*** Search: _setIcon: Setting icon url "data:image/png;base64,AAABAAEAEBAAAAEAGABoAwAAFgAAACgAAAAQAAAAIAAAAAEAGAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAADs9Pt8xetPtu9FsfFNtu%2BTzvb2%2B%2Fne4dFJeBw0egA%2FfAJAfAA8ewBBegAAAAD%2B%2FPtft98Mp%2BwWsfAVsvEbs%2FQeqvF8xO7%2F%2F%2F63yqkxdgM7gwE%2FggM%2BfQA%2BegBDeQDe7PIbotgQufcMufEPtfIPsvAbs%2FQvq%2Bfz%2Bf%2F%2B%2B%2FZKhR05hgBBhQI8hgBAgAI9ewD0%2B%2Fg3pswAtO8Cxf4Kw%2FsJvvYAqupKsNv%2B%2Fv7%2F%2FP5VkSU0iQA7jQA9hgBDgQU%2BfQH%2F%2Ff%2FQ6fM4sM4KsN8AteMCruIqqdbZ7PH8%2Fv%2Fg6Nc%2Fhg05kAA8jAM9iQI%2BhQA%2BgQDQu6b97uv%2F%2F%2F7V8Pqw3eiWz97q8%2Ff%2F%2F%2F%2F7%2FPptpkkqjQE4kwA7kAA5iwI8iAA8hQCOSSKdXjiyflbAkG7u2s%2F%2B%2F%2F39%2F%2F7r8utrqEYtjQE8lgA7kwA7kwA9jwA9igA9hACiWSekVRyeSgiYSBHx6N%2F%2B%2Fv7k7OFRmiYtlAA5lwI7lwI4lAA7kgI9jwE9iwI4iQCoVhWcTxCmb0K%2BooT8%2Fv%2F7%2F%2F%2FJ2r8fdwI1mwA3mQA3mgA8lAE8lAE4jwA9iwE%2BhwGfXifWvqz%2B%2Ff%2F58u%2Fev6Dt4tr%2B%2F%2F2ZuIUsggA7mgM6mAM3lgA5lgA6kQE%2FkwBChwHt4dv%2F%2F%2F728ei1bCi7VAC5XQ7kz7n%2F%2F%2F6bsZkgcB03lQA9lgM7kwA2iQktZToPK4r9%2F%2F%2F9%2F%2F%2FSqYK5UwDKZAS9WALIkFn%2B%2F%2F3%2F%2BP8oKccGGcIRJrERILYFEMwAAuEAAdX%2F%2Ff7%2F%2FP%2B%2BfDvGXQLIZgLEWgLOjlf7%2F%2F%2F%2F%2F%2F9QU90EAPQAAf8DAP0AAfMAAOUDAtr%2F%2F%2F%2F7%2B%2Fu2bCTIYwDPZgDBWQDSr4P%2F%2Fv%2F%2F%2FP5GRuABAPkAA%2FwBAfkDAPAAAesAAN%2F%2F%2B%2Fz%2F%2F%2F64g1C5VwDMYwK8Yg7y5tz8%2Fv%2FV1PYKDOcAAP0DAf4AAf0AAfYEAOwAAuAAAAD%2F%2FPvi28ymXyChTATRrIb8%2F%2F3v8fk6P8MAAdUCAvoAAP0CAP0AAfYAAO4AAACAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACAAQAA" for engine "Test search engine".
*** Search: NOTIFY: Engine: "Test search engine"; Verb: "engine-changed"

TEST-INFO | /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_nocache.js | Observing topic browser-search-engine-modified
*** Search: NOTIFY: Engine: "Test search engine"; Verb: "engine-loaded"

TEST-INFO | /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_nocache.js | Observing topic browser-search-engine-modified
*** Search: nsSearchService::observe: Done installation of Test search engine.
*** Search: _addEngineToStore: Adding engine: "Test search engine"
*** Search: NOTIFY: Engine: "Test search engine"; Verb: "engine-added"

TEST-INFO | /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_nocache.js | Observing topic browser-search-engine-modified

TEST-INFO | /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_nocache.js | Engine has been added, let's wait for the cache to be built
*** Search: nsSearchService::observe: setting current
*** Search: _buildSortedEngineList: building list
*** Search: NOTIFY: Engine: "Test search engine"; Verb: "engine-current"
*** Search: _batchCacheInvalidation: Batch timer reset

TEST-INFO | /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/tests/toolkit/components/search/tests/xpcshell/head_search.js | afterCache: write-cache-to-disk-complete

TEST-INFO | /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_nocache.js | Success

TEST-INFO | /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_nocache.js | Searching test engine in cache

TEST-INFO | /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/tests/toolkit/components/search/tests/xpcshell/head_search.js | afterCache: write-cache-to-disk-complete

TEST-PASS | /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_nocache.js | [cacheCreated : 40] true == true

TEST-UNEXPECTED-FAIL | /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_nocache.js | false == true - See following stack:
JS frame :: /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/head.js :: do_throw :: line 461
JS frame :: /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/head.js :: do_report_result :: line 563
JS frame :: /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/head.js :: _do_check_eq :: line 573
JS frame :: /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/head.js :: do_check_eq :: line 580
JS frame :: /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/head.js :: do_check_true :: line 594
JS frame :: /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_nocache.js :: task :: line 79
JS frame :: resource://gre/modules/Task.jsm :: TaskImpl_run :: line 192
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 120
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: then :: line 45
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 187
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 120
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: then :: line 45
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 187
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 120
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: then :: line 45
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 187
JS frame :: resource://gre/modules/osfile/_PromiseWorker.jsm :: onmessage :: line 134
native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0

TEST-INFO | (xpcshell/head.js) | exiting test

TEST-UNEXPECTED-FAIL | /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_nocache.js | 2147500036 - See following stack:
JS frame :: /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/head.js :: do_throw :: line 461
JS frame :: /tmp/tmp.pFFfKlLDYi/dsc0t-xpcshell-tests-testtmp/tmpdir/tmpB9hGWd/xpcshell/tests/toolkit/components/search/tests/xpcshell/test_nocache.js :: task :: line 81
JS frame :: resource://gre/modules/Task.jsm :: TaskImpl_run :: line 192
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 120
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: then :: line 45
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 187
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 120
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: then :: line 45
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 187
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 120
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: then :: line 45
JS frame :: resource://gre/modules/commonjs/sdk/core/promise.js :: resolve :: line 187
JS frame :: resource://gre/modules/osfile/_PromiseWorker.jsm :: onmessage :: line 134
native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0

TEST-INFO | (xpcshell/head.js) | exiting test

TEST-INFO | (xpcshell/head.js) | test 1 finished

TEST-INFO | (xpcshell/head.js) | exiting test
<<<<<<<
Flags: needinfo?(chrisccoulson)

          You need to log in
          before you can comment on or make changes to this bug.