Skip to content

Conversation

@TobiGr
Copy link
Contributor

@TobiGr TobiGr commented Jan 2, 2022

  • I carefully read the contribution guidelines and agree to them.
  • I have tested the API against NewPipe.
  • I agree to create a pull request for NewPipe as soon as possible to make it compatible with the changed API.

Update our fork of nanojson. This includes a few dependency updates as well as smaller improvements from upstream.

@TobiGr TobiGr added the dependencies Pull requests that update a dependency file label Jan 2, 2022
Copy link
Member

@litetex litetex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 points I have to discuss here:

  1. All tests are failing there seems to be something wrong
  2. What are the changes in the new "version"?

@FireMasterK
Copy link
Member

FireMasterK commented Sep 5, 2022

I just want to add that the current fork of nanojson uses significantly more CPU than upstream!

In Piped, I'm now using a personal fork since TeamPiped/Piped-Backend#357, the difference is MASSIVE!

I did this after seeing that nanojson was using a lot of CPU in the nextToken method. The NewPipe fork has changes, calling this method more times (for JS parsing?), which could be the cause of this.

I don't have a before screenshot, but here's the after (it was ~1800 seconds before, after 10 minutes of recording/profiling):
After:
image
nanojson is nowhere close to the top with this!

@opusforlife2
Copy link
Collaborator

@FireMasterK If the only difference between this version and your fork is upstream changes, could they not be added to this fork?

@FireMasterK
Copy link
Member

@FireMasterK If the only difference between this version and your fork is upstream changes, could they not be added to this fork?

I suppose they could, but I only tested it on YouTube (in Piped), I'm not sure if other services require the JS json extraction. In my fork, I have the upstream changes and have cherry-picked only this commit: TeamNewPipe/nanojson@d5da581

@Stypox
Copy link
Member

Stypox commented Nov 24, 2022

Closing in favor of #981. Thanks @FireMasterK for the heads-up!

@Stypox Stypox closed this Nov 24, 2022
@Stypox Stypox mentioned this pull request Nov 24, 2022
3 tasks
@TobiGr TobiGr deleted the dependencies/nanojson branch November 24, 2022 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants