Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: reduce package file size #1730

Closed
wants to merge 4 commits into from
Closed

feat: reduce package file size #1730

wants to merge 4 commits into from

Conversation

bshaffer
Copy link
Contributor

@bshaffer bshaffer commented Nov 21, 2022

Reduces the package file size by doing the following:

  • Puts PHP docs on one line
  • Puts all model files in model.php

Testing on the branch consolidate-models-gen, with just these two changes, we get a reduction of 99M to 44M:

$ composer require google/apiclient-services 
Using version ^0.275.0 for google/apiclient-services
$ du -sh vendor/google/apiclient-services
 99M	vendor/google/apiclient-services

$ composer require google/apiclient-services:dev-consolidate-models-gen
$ du -sh  vendor/google/apiclient-services
 44M	vendor/google/apiclient-services

That's a reduction of 56% file size (55MB)!!

TODO: Verify what composer pulls down and check the size there

NOTE: My testing removed RemoteBuildExecution, SemanticTile, AdExchangeBuyer, Webmasters, and PlayableLocations

Other consideration

  • When protected $xyzDataType = '', don't create the property, as it's functionally equivalent to the property not existing (see Model::__get).
  • Remove class_alias calls (they've been around for a year and a half now)

Other OTHER considerations (not related to package file size)

  • Move Service classes inside Service directory
  • Add an "unofficial" mirror repo for creating the composer packages (e.g. googlemirror/apiclient-mybusinessqa)

@bshaffer bshaffer requested review from a team and yoshi-approver as code owners November 21, 2022 23:19
@conventional-commit-lint-gcf
Copy link

conventional-commit-lint-gcf bot commented Nov 21, 2022

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@bshaffer bshaffer marked this pull request as draft November 21, 2022 23:20
@ejunker
Copy link

ejunker commented Mar 29, 2023

I could really use this. I am only using 1 Google service and I am using an NFS mounted filesystem that is shared between my web servers. Sometimes when installing an updated version of this package I run into a timeout because it takes so long to write all these small files. I know that is not your problem but reducing the size and number of files would be beneficial to many people.

@bshaffer
Copy link
Contributor Author

bshaffer commented Apr 4, 2023

Hello @ejunker !
On second thought, reducing the package size by removing things like repeated blocks of code probably won't reduce the time to install the package. This is because git uses compression algorithms which should already be optimized to take care of such things.
If you can definitively say the changes in this PR solve your issue, then I'm happy to merge and release this. However, unless we are confident this would alleviate the issue, I think we need to look for a better solution.

Please test using this branch (you can do so by running composer require google/apiclient-services:dev-consolidate-models-gen on your NFS) and let me know if you see a substantial difference. Thank you!

@ejunker
Copy link

ejunker commented Apr 4, 2023

Here are some benchmarks I did:
original version: 6 minutes 22 seconds
your consolidate-models-gen branch: 1 minute 10 seconds

I ended up creating my own fork of the package with only the files I need. When I install that package it only takes 2 seconds!

This is what my composer.json looks like:

    "require": {
        "google/apiclient-services": "dev-main as v0.292.0"
    },
    "repositories": [
        {
            "type": "vcs",
            "url": "git@github.com:ejunker/google-indexing.git"
        }
    ]

I think I will continue to use my custom fork but it is encouraging to see it drop from 6 minutes down to 1 minute.

@ejunker
Copy link

ejunker commented Apr 4, 2023

Have you ever thought about creating read-only subtree splits and publishing multiple packages? Many of the PHP frameworks like Laravel and Symfony do this. https://tomasvotruba.com/blog/2017/01/31/how-monolithic-repository-in-open-source-saved-my-laziness/

@bshaffer
Copy link
Contributor Author

bshaffer commented Apr 4, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants