Closed Bug 1686031 Opened 3 years ago Closed 3 years ago

Simplify structures and naming related to directory metadata

Categories

(Core :: Storage: Quota Manager, task, P2)

Product:
Core
Component:
Storage: Quota Manager
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
88 Branch
Tracking Flags:
Tracking Status
firefox88 --- fixed

People

(Reporter: janv, Assigned: janv)

References

(Blocks 1 open bug)

Details

Attachments

(16 files)

Bug 1686031 - Remove QuotaManager::GetDirectoryMetadata2 and QuotaManager::GetDirectoryMetadata2WithRestore; r=#dom-workers-and-storage
48 bytes, text/x-phabricator-request
Details | Review
Bug 1686031 - Rename GroupAndOrigin to OriginMetadata; r=#dom-storage
48 bytes, text/x-phabricator-request
Details | Review
Bug 1686031 - Fold QuotaInfo into OriginMetadata and rename QuotaInfo.h to OriginMetadata.h; r=#dom-storage
48 bytes, text/x-phabricator-request
Details | Review
Bug 1686031 - Add suffix to OriginInfoPair and quota info cache; r=#dom-storage
48 bytes, text/x-phabricator-request
Details | Review
Bug 1686031 - Construct OriginMetadata with real suffix in GetOriginUsageOp::DoDirectoryWork; r=#dom-storage
48 bytes, text/x-phabricator-request
Details | Review
Bug 1686031 - Construct OriginMetadata with real suffix in PersistedOp::DoDirectoryWork; r=#dom-storage
48 bytes, text/x-phabricator-request
Details | Review
Bug 1686031 - Construct OriginMetadata with real suffix in DirectoryLockImpl::OriginMetadata; r=#dom-storage
48 bytes, text/x-phabricator-request
Details | Review
Bug 1686031 - Change SaveCacheVersion, CreateCacheTables and UpgradeCacheFrom1To2 to take a reference instead of a raw pointer; r=#dom-storage
48 bytes, text/x-phabricator-request
Details | Review
Bug 1686031 - Move mGroup from GroupInfo to GroupInfoPair; r=#dom-storage
48 bytes, text/x-phabricator-request
Details | Review
Bug 1686031 - Introduce OriginInfo::FlattenToOriginMetadata and make use of it; r=#dom-storage
48 bytes, text/x-phabricator-request
Details | Review
Bug 1686031 - Add mPersistenceType to OriginMetadata; r=#dom-storage
48 bytes, text/x-phabricator-request
Details | Review
Bug 1686031 - Fix GetDirectoryMetadataWithOriginMetadata2 and GetDirectoryMetadataWithOriginMetadata2WithRestore to return proper persistence type; r=#dom-storage
48 bytes, text/x-phabricator-request
Details | Review
Bug 1686031 - Fix storage upgrades and restoration of the metadata file to use proper persistence type internally; r=#dom-storage
48 bytes, text/x-phabricator-request
Details | Review
Bug 1686031 - Replace GetDirectoryResultWithOriginMetadata with FullOriginMetadata; r=#dom-storage
48 bytes, text/x-phabricator-request
Details | Review
Bug 1686031 - Rename GetDirectoryMetadataWithOriginMetadata2 to LoadFullOriginMetadata and GetDirectoryMetadataWithOriginMetadata2WithRestore to LoadFullOriginMetadataWithRestore; r=#dom-storage
48 bytes, text/x-phabricator-request
Details | Review
Bug 1686031 - Rename OriginMetadata.h to CommonMetadata.h; r=#dom-storage
48 bytes, text/x-phabricator-request
Details | Review

This is a follow-up for bug 1679541.

First of all, these two functions can be removed:
QuotaManager::GetDirectoryMetadata2
QuotaManager::GetDirectoryMetadata2WithRestore

Those functions are similar to GetDirectoryMetadataWithQuotaInfo2 and GetDirectoryMetadataWithQuotaInfo2WithRestore, but they only return the timestamp and the persisted flag. It was kind of an optimization to read only necessary data for specific cases.
However, the underlying streams will read entire block from the disk anyway and the additional reads from memory buffers don't cost much.
There's also a question, if partial reading of the metadata file is actually safe. The metadata file can have other records corrupted, so the file should be completely ignored and restored, because we can't trust that the timestamp and the flag is ok.
The "optimized" version of directory metadata is only used for the persistent repository which is not used much which makes it even less interesting for doing optimizations like this.

Then there's the naming of data structures and methods which is currently a bit confusing and complicated.

There are already structures which are all related to groups and origins:

  1. GroupInfoPair
  • owned by QuotaManager
  • keyed by the group name
  • owns two GroupInfo objects, one for the default repository and one for the temporary repository
  1. GroupInfo
  • owned by GroupInfoPair
  • represents the group for given repository
  • owns OriginInfo objects, there can be multiple OriginInfo objects for given group
  1. OriginInfo
  • owned by GroupInfo,
  • represents the origin for given repository and group
  • references QuotaObject objects (a bit tricky, using weak pointers)
  1. QuotaObject
  • referenced by OriginInfo
  • represents a quota tracked file given repository, group and origin

I don't want to change names of those objects, at least for now.

Then we have:

struct GroupAndOrigin {
  nsCString mGroup;
  nsCString mOrigin;
};

struct QuotaInfo : GroupAndOrigin {
  nsCString mSuffix;
}

These were introduced only recently to reduce number of parameters of many functions.

Bug 1679541 also introduced:

struct GetDirectoryResult {
    int64_t mTimestamp;
    bool mPersisted;
};

struct GetDirectoryResultWithQuotaInfo : GetDirectoryResult {
   QuotaInfo mQuotaInfo;
};

The first one can be removed because the related method can also be removed as I proposed above.

GetDirectoryResultWithQuotaInfo can then be renamed to DirectoryInfo, inherit from QuotaInfo and add the timestamp and the persisted flag:

struct DirectoryInfo : QuotaInfo {
   int64_t mTimestamp;
   bool mPersisted;
}

DirectoryInfo represents a flat view of OriginInfo which is part of the in-memory tree structure (QuotaManager -> GroupInfoPair -> ...)

GetDirectoryMetadataWithQuotaInfo2 and GetDirectoryMetadataWithQuotaInfo2WithRestore can also be renamed to
GetDirectoryInfoandGetDirectoryInfoWithRestore` (but only those in QuotaManager, the ones related to storage upgrades should stay unchanged, at least for now)

It would be nice to find better names for GroupAndOrigin and QuotaInfo, I'll see if there's something more suitable.

Assignee: nobody → jvarga

When we improve naming of the GetDirectoryMetadata* functions, we should also consider not using Get as a prefix: To start with, these are no getters in a stricter sense, given they are reading from a file. GetDirectoryMetadataWithQuotaInfo2* even writes to the metadata file. So maybe MaybeUpdateDirectoryInfo and MaybeRestoreAndUpdateDirectoryInfo are better names? The fact that they also return the directory info can be neglected in the name IMO.

When replacing uses of the non-WithQuotaInfo versions with the WithQuotaInfo versions of the functions, this not only is less efficient because more data is read from the file and returned to the call, which might be negligible, but it also involves more fallible functions, in particular overwriting the metadata file might fail. Just confirming that I understand you correctly that this is actually desirable to improve consistency?

I think when this is sorted out, a comment should list only the actions that should be taken to complete this bug, without the reasoning behind it. The current description is somewhat complex.

(In reply to Simon Giesecke [:sg] [he/him] from comment #1)

When we improve naming of the GetDirectoryMetadata* functions, we should also consider not using Get as a prefix: To start with, these are no getters in a stricter sense, given they are reading from a file. GetDirectoryMetadataWithQuotaInfo2* even writes to the metadata file. So maybe MaybeUpdateDirectoryInfo and MaybeRestoreAndUpdateDirectoryInfo are better names? The fact that they also return the directory info can be neglected in the name IMO.

Ok, will consider that.

When replacing uses of the non-WithQuotaInfo versions with the WithQuotaInfo versions of the functions, this not only is less efficient because more data is read from the file and returned to the call, which might be negligible, but it also involves more fallible functions, in particular overwriting the metadata file might fail. Just confirming that I understand you correctly that this is actually desirable to improve consistency?

Yes, consistency too. I think it was over-optimized. It's true that those functions are fallible, but before they are called (or rather during the first call) all data is read in one block from the disk, and size of the data is smaller than the block size.

I think when this is sorted out, a comment should list only the actions that should be taken to complete this bug, without the reasoning behind it. The current description is somewhat complex.

There will be separate patches with more concrete descriptions.

Callers of these functions have been changed to call
GetDirectoryMetadataWithQuotaInfo2 and
GetDirectoryMetadataWithQuotaInfo2WithRestore instead which fully load the
metadata file.

(In reply to Jan Varga [:janv] from comment #2)

(In reply to Simon Giesecke [:sg] [he/him] from comment #1)

I think when this is sorted out, a comment should list only the actions that should be taken to complete this bug, without the reasoning behind it. The current description is somewhat complex.

There will be separate patches with more concrete descriptions.

When I asked for that, I thought I would be implementing this. If you are doing it yourself, we don't need a list here :)

I'll see how it goes, we might file separate bugs for this and work on it in parallel.

Ok, I think I now have a good plan for this, but it touches many files.
Simon, my patches are based on D101183 (as you suggested), but if you plan any big refactorings in QM or quota clients, please wait a bit.
It also means that I need to review everything prior D101183 before this can land.

(In reply to Jan Varga [:janv] from comment #6)

Ok, I think I now have a good plan for this, but it touches many files.
Simon, my patches are based on D101183 (as you suggested), but if you plan any big refactorings in QM or quota clients, please wait a bit.
It also means that I need to review everything prior D101183 before this can land.

As discussed on Slack, I'll abstain from doing additional refactorings, but there are a lot of them in flight right now, also outside the stack containing D101183.

Keywords: leave-open
Pushed by jvarga@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5793732d3285
Remove QuotaManager::GetDirectoryMetadata2 and QuotaManager::GetDirectoryMetadata2WithRestore; r=dom-workers-and-storage-reviewers,sg
Blocks: 1671932

(In reply to Jan Varga [:janv] from comment #0)

It would be nice to find better names for GroupAndOrigin and QuotaInfo, I'll see if there's something more suitable.

I changed the plan a bit and as a first step, we need to rename GroupAndOrigin to OriginMetadata and then merge it with QuotaInfo. The suffix is currently not needed in some places, but that is going to change in following patches. Even if it's not used in a few places, I think it's still better to have a unified structure which makes it all much simpler and cleaner.

I'm going to submit two patches and OriginMetadata will look like this after that:

struct OriginMetadata {
  nsCString mSuffix;
  nsCString mGroup;
  nsCString mOrigin;

  OriginMetadata() = default;

  OriginMetadata(nsCString aSuffix, nsCString aGroup, nsCString aOrigin)
      : mSuffix{std::move(aSuffix)},
        mGroup{std::move(aGroup)},
        mOrigin{std::move(aOrigin)} {}
};

The next step will be the addition of mPersistenceType to OriginMetadata

And finally the change of:

struct GetDirectoryResultWithOriginMetadata {
    int64_t mTimestamp;
    bool mPersisted;
    OriginMetadata mOriginMetadata;
};

to:

struct FullOriginMetadata : OriginMetadata {
   int64_t mTimestamp;
   bool mPersisted;
}

We can figure out better method names after all these changes.

Pushed by jvarga@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/840c3738d147
Rename GroupAndOrigin to OriginMetadata; r=dom-storage-reviewers,sg
Pushed by jvarga@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/57484ffab666
Fold QuotaInfo into OriginMetadata and rename QuotaInfo.h to OriginMetadata.h; r=dom-storage-reviewers,sg

This fixes the two spots where we were always passing an empty suffix to
OriginMetadata.

Depends on D106019

The existing members of OriginMetadata have been extracted to a parent struct
called PrincipalMetadata. Methods like GetOriginUsage,
GetInfoFromValidatedPrincipalInfo, GetInfoFromPrincipal and GetInfoForChrome
have been changed to take/return PrincipalInfo instead of OriginMetadata.
Having the persistence type doesn't make sense in those methods because the
origin is not tight to a specific persistence type in context of the methods.

Some places temporarily pass PERSISTENCE_TYPE_INVALID and will be fixed in
following patches.

Depends on D106399

Pushed by jvarga@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6a2d82b48f78
Add suffix to OriginInfoPair and quota info cache; r=dom-storage-reviewers,sg
https://hg.mozilla.org/integration/autoland/rev/93933a2da1a7
Construct OriginMetadata with real suffix in GetOriginUsageOp::DoDirectoryWork; r=dom-storage-reviewers,sg
https://hg.mozilla.org/integration/autoland/rev/7f46c92e1833
Construct OriginMetadata with real suffix in PersistedOp::DoDirectoryWork; r=dom-storage-reviewers,sg
https://hg.mozilla.org/integration/autoland/rev/4f7704ba36ed
Construct OriginMetadata with real suffix in DirectoryLockImpl::OriginMetadata; r=dom-storage-reviewers,sg
https://hg.mozilla.org/integration/autoland/rev/7c04f2a9a365
Change SaveCacheVersion, CreateCacheTables and UpgradeCacheFrom1To2 to take a reference instead of a raw pointer; r=dom-storage-reviewers,sg
https://hg.mozilla.org/integration/autoland/rev/e14e574f9f4b
Move mGroup from GroupInfo to GroupInfoPair; r=dom-storage-reviewers,sg
https://hg.mozilla.org/integration/autoland/rev/336c31529dbb
Introduce OriginInfo::FlattenToOriginMetadata and make use of it; r=dom-storage-reviewers,sg
Pushed by jvarga@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bc41e3cc6754
Add mPersistenceType to OriginMetadata; r=dom-storage-reviewers,sg
Pushed by jvarga@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c69b2c1ef6f9
Fix GetDirectoryMetadataWithOriginMetadata2 and GetDirectoryMetadataWithOriginMetadata2WithRestore to return proper persistence type; r=dom-storage-reviewers,sg
Pushed by jvarga@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/470d617d7673
Fix storage upgrades and restoration of the metadata file to use proper persistence type internally; r=dom-storage-reviewers,sg
https://hg.mozilla.org/integration/autoland/rev/9be2fd0b5b6a
Replace GetDirectoryResultWithOriginMetadata with FullOriginMetadata; r=dom-storage-reviewers,sg
https://hg.mozilla.org/integration/autoland/rev/43a39c491548
Rename GetDirectoryMetadataWithOriginMetadata2 to LoadFullOriginMetadata and GetDirectoryMetadataWithOriginMetadata2WithRestore to LoadFullOriginMetadataWithRestore; r=dom-storage-reviewers,sg
Attachment #9207777 - Attachment description: Bug 1686031 - Rename OriginMetadata.h to Metadata.h; r=#dom-storage → Bug 1686031 - Rename OriginMetadata.h to CommonMetadata.h; r=#dom-storage
Pushed by jvarga@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/913f16ccb065
Fix storage upgrades and restoration of the metadata file to use proper persistence type internally; r=dom-storage-reviewers,sg
Pushed by jvarga@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9ef8072221e8
Replace GetDirectoryResultWithOriginMetadata with FullOriginMetadata; r=dom-storage-reviewers,sg
https://hg.mozilla.org/integration/autoland/rev/f5f020daba36
Rename GetDirectoryMetadataWithOriginMetadata2 to LoadFullOriginMetadata and GetDirectoryMetadataWithOriginMetadata2WithRestore to LoadFullOriginMetadataWithRestore; r=dom-storage-reviewers,sg
Keywords: leave-open
Pushed by jvarga@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1de71b8527c2
Rename OriginMetadata.h to CommonMetadata.h; r=dom-storage-reviewers,sg

https://hg.mozilla.org/mozilla-central/rev/1de71b8527c2

Status: NEW → RESOLVED
Closed: 3 years ago
status-firefox88: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 88 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: